pulsejet / memories

Fast, modern and advanced photo management suite. Runs as a Nextcloud app.
https://memories.gallery
GNU Affero General Public License v3.0
2.86k stars 77 forks source link

occ memories:places-setup do not work with no table prefix #648

Closed diroots closed 1 year ago

diroots commented 1 year ago

Describe the bug occ memories places:setup do not work with a nextcloud installation with empty table prefix.

our installation is done with this parameter in config.php : 'dbtableprefix' => '',

when launching occ memories places:setup, we get this result :

server:/path/to/nextcloud# occ memories:places-setup
Attempting to set up reverse geocoding

In Places.php line 42:

  Database table prefix is not set. Cannot use database extensions (dbtableprefix).  

memories:places-setup

it's a bit like https://github.com/pulsejet/memories/issues/632, except my nc installation is with empty table prefix, and i cannot change it,...

To Reproduce

Platform:

Additional context Add any other context about the problem here.

diroots commented 1 year ago

also i don't get the purpose of this check : https://github.com/pulsejet/memories/blob/master/lib/Service/Places.php#L41

as when checking where is used this $prefix variable the the whole app, it's only called there :

root@server:/path/to/nextcloud/html/custom_apps/memories# grep -nR --exclude-dir=exiftool-bin '$prefix'
lib/Service/Places.php:40:        $prefix = $this->config->getSystemValue('dbtableprefix', '') ?: '';
lib/Service/Places.php:41://        if ('' === $prefix) {
root@server:/path/to/nextcloud/html/custom_apps/memories# 

so it's defined, then checked for not being empty, but then it's not used anywhere.

why is this check needed, then? and it should be tested that it is defined correctly, but it can be empty, as nextcloud install can have prefix empty (https://github.com/nextcloud/server/blob/master/config/config.sample.php#L145).

pulsejet commented 1 year ago

Unfortunately there's no easy way around this. Doctrine doesn't play well with unknown column types when calculating the schema, so adding a geometry column in the same namespaces causes it to crash. Putting nextcloud's tables inside a prefix allows placing the geometry table in a separate memories_ namespace, which Doctrine then ignores.

pulsejet commented 1 year ago

The sample config has been corrected now: https://github.com/nextcloud/server/pull/38321

diroots commented 1 year ago

but what happens with a nc instance already set up without prefix (running for years)? we cannot install memories? like there is no other choice?

akhil1508 commented 1 year ago

Putting nextcloud's tables inside a prefix allows placing the geometry table in a separate memories_ namespace, which Doctrine then ignores.

@pulsejet In an installation which had dbtableprefix => 'oc_', I could see tables like oc_memories_places and oc_memories. How does Doctrine ignore these?

Update: NVM, I got it. Only the memories_planet_geometry is outside the prefix.

pulsejet commented 1 year ago

but what happens with a nc instance already set up without prefix (running for years)? we cannot install memories? like there is no other choice?

Places is an optional feature, so you should be able to use everything else just fine. If you really need places support, you'll need to manually migrate the DB to a non blank prefix (basically rename all tables to have this prefix, then update config.php)

bendschs commented 9 months ago

so i guess this is why i am getting the following error message?

Attempting to set up reverse geocoding
Database support was detected
Download planet data to temporary file...
Inserting planet data into database...

In Places.php line 442:

  Failed to create database tables: An exception occurred while executing a q  
  uery: SQLSTATE[42000]: Syntax error or access violation: 1071 Specified key  
   was too long; max key length is 1000 bytes                                  
pulsejet commented 9 months ago

@bendschs that seems unrelated. You might need to upgrade / set some options in your DB. What DB is this anyway?

bendschs commented 9 months ago

it's MariaDB 10.5.8. reading over various issues regarding the planet database it seems almost like this feature is still a bit experimental (not to be taken as criticism)?

thanks for the amazing work you have done at this point, it's really quite impressive how much progress you made with this app, it's already so much better than the official photos app.

pulsejet commented 9 months ago

it's MariaDB 10.5.8. reading over various issues regarding the planet database it seems almost like this feature is still a bit experimental (not to be taken as criticism)?

It's quite stable actually. Two issues that are common:

  1. Memory limits: batch insertions are faster but they end up creating some trouble on low memory systems.
  2. This (#648) issue, which nothing can be done about (it's a Doctrine limitation). Fortunately, any Nextcloud setup from the last five years or so likely shouldn't have trouble.

Regarding the error you got (first time I'm seeing this), it's likely a database configuration mismatch; what's happening here is MariaDB is refusing to create the spatial index. A couple of things to try:

  1. occ maintenance:repair
  2. You could verify some db/table settings
    • You are using InnoDB
    • Row format is dynamic
    • Collation is utf8mb4_general_ci

thanks for the amazing work you have done at this point, it's really quite impressive how much progress you made with this app, it's already so much better than the official photos app.

šŸŽ‰

bendschs commented 9 months ago

Thank you for the immediate reply. I've noticed, that my collation is utf8mb4_bin instead of utf8mb4_general_ci. do you think it would make a difference changing the collation to utf8mb4_general_ci for all the tables?

row format is dynamic and i am usind InnoDB.

pulsejet commented 9 months ago

Thank you for the immediate reply. I've noticed, that my collation is utf8mb4_bin instead of utf8mb4_general_ci. do you think it would make a difference changing the collation to utf8mb4_general_ci for all the tables?

Nope, seems unlikely. Out of ideas at this point, a couple of pointers:

  1. Check for innodb_large_prefix, the table should be created with it on, I believe.
  2. How are you deploying the DB? A fresh Docker install seems to work so might be worth checking the difference in the config.
  3. You could try commenting out the index in lib/Service/Places.php lines 433 - 437 to make sure that the spatial index is throwing the error and not something else. Also reduce the size of varchar above, but this should make no difference likely, because they aren't indexed.
pulsejet commented 9 months ago

Also reduce the size of varchar above, but this should make no difference likely, because they aren't indexed.

I realised there is an index on this (the pkey) so this is definitely worth trying. Try reducing the varchar(255) to varchar(128) or 64.

pulsejet commented 9 months ago

@bendschs this change: https://github.com/pulsejet/memories/commit/5cffbf2197b482196dfb256ff29bc54d0bdb89d1

bendschs commented 9 months ago

that did the trick, thank you so much šŸ™

rhyst commented 8 months ago

Just in case it is relevant for anyone searching I also had the Database table prefix is not set message despite a relatively modern nextcloud installation. I checked the database and could see all the tables were actually prefixed with oc_. I checked my config.php and it was missing a dbtableprefix entry so I added one:

'dbtableprefix' => 'oc_',

And places-setup now works. Not sure how I ended up without that config item but with prefixes.

Glandos commented 1 month ago

I have an old installation (migrated from ownCloud actually), and this hit me.

If there's no workaround, maybe it's good to have an entry in the FAQ?

pulsejet commented 1 month ago

If there's no workaround, maybe it's good to have an entry in the FAQ?

I've added an entry to the docs. https://memories.gallery/troubleshooting/#database-table-prefix

Technically the workaround is to rename all tables to have a prefix. Theoretically this is easy but I've not tried it.

e-minguez commented 1 month ago

Will this work? https://help.nextcloud.com/t/rename-database-and-prefixes/28296/4 (I mean, running the opposite, adding the prefix)

pulsejet commented 1 month ago

Yeah, it should theoretically work. Strongly suggest having backups.