Schema improvements to better support MySQL. Rewrite the MySQL database schema to remove the need for SUPER privileges when creating/amending the digikam database. Use foreign key referential integrity to achieve the behaviour of the triggers. Specifically use ON DELETE to cascade deletion of a Tag, Image, Album or AlbumRoot any items that depend upon it. This change does make the database less tolerant of bad data that references non-existant entries. But that can be argued to be a good thing because it causes a fast fail in the case of bugs/issues that would have otherwise caused silent data loss. The schema changes are not fully completed yet and is intended as a proof of concept patch series. Comments most welcome! Notes: Testing performed using the following MySQL setup. GRANT USAGE ON *.* TO 'digiuser'@'localhost' IDENTIFIED BY 'abc123'; GRANT ALL PRIVILEGES ON `digikam`.* TO 'digiuser'@'localhost'; CREATE DATABASE digikam; The conversion code was failing to catch SQL errors in the migration process. I corrected this by returning an error if a query fails but it is not clear if this is the intended behaviour and it maybe that this will uncover other issues both in SQLite and MySQL. The Albums table has an icon field that causes a circular reference to the Images table. This needs special handling both during table deletion and data migration. I put a hack in place for the table deletion case but did not address the migration where images will initially need creating without the icon being set and then the icon values need copying over once the Images have been migrated. The migration will fail if any orphan records exist that do not reference an existing AlbumRoot, Album, Image or Tag. Normally that would be due to some previous uncaught error due to failed creation or partial deletion. I did notice that in my newly created SQLite database with only a few test images the Tags "icon" field was populated with a zero instead of a null. This causes the migration to fail. I worked around this by executing the following against the SQLite database. update Tags set icon = null where icon = 0; The thumbnail and face schema have not been fully examined/converted yet. The triggers have been disabled to allow migration testing. Similarly I have not looked at database upgrade from the previous version of the MySQL schema. There is at least one user (myself) who has been using MySQL so having a proper upgrade to add the referential integrity would seem to be needed in a final solution. Reproducible: Always Steps to Reproduce: 1. Proof of concept patch series
Created attachment 95686 [details] [PATCH 0/5] MySQL schema improvements
Created attachment 95687 [details] [PATCH 1/5] Ensure that an error is returned if the query fails.
Created attachment 95688 [details] [PATCH 2/5] Delete tables in reverse creation order.
Created attachment 95689 [details] [PATCH 3/5] Ensure that data is migrated in line with foreign key
Created attachment 95690 [details] [PATCH 4/5] Remove trigger dependency for MySQL.
Created attachment 95691 [details] [PATCH 5/5] Temporary workaround to break MySQL references loop with the Albums icon entry.
Git commit 38cb75f52bc7f602fe75d188106ffb8f86a4d568 by Gilles Caulier. Committed on 24/11/2015 at 12:02. Pushed by cgilles into branch 'master'. apply patch #95687 to return an error if SQL query fails. M +4 -0 libs/database/engine/dbenginebackend.cpp http://commits.kde.org/digikam/38cb75f52bc7f602fe75d188106ffb8f86a4d568
Git commit 4853eedeac43d4a7c59942e40153482f7732e150 by Gilles Caulier. Committed on 24/11/2015 at 12:34. Pushed by cgilles into branch 'master'. apply patch #95689 to be able to use of referential integrity checks in the database schema M +2 -2 libs/database/coredb/coredbcopymanager.cpp http://commits.kde.org/digikam/4853eedeac43d4a7c59942e40153482f7732e150
Comment on attachment 95686 [details] [PATCH 0/5] MySQL schema improvements >From 8cfb15da5f4de65a734aa864c68871822e144de1 Mon Sep 17 00:00:00 2001 >From: Richard Mortimer <richm@oldelvet.org.uk> >Date: Tue, 24 Nov 2015 08:56:22 +0000 >Subject: [PATCH 0/5] MySQL schema improvements > >Rewrite the MySQL database schema to remove the need for SUPER privileges >when creating/amending the digikam database. > >Use foreign key referential integrity to achieve the behaviour of the >triggers. Specifically use ON DELETE to cascade deletion of a Tag, Image, >Album or AlbumRoot any items that depend upon it. > >This change does make the database less tolerant of bad data that references >non-existant entries. But that can be argued to be a good thing because >it causes a fast fail in the case of bugs/issues that would have otherwise >caused silent data loss. > >The schema changes are not fully completed yet and is intended as a proof of >concept patch series. Comments most welcome! > >Notes: > >Testing performed using the following MySQL setup. > >GRANT USAGE ON *.* TO 'digiuser'@'localhost' IDENTIFIED BY 'abc123'; >GRANT ALL PRIVILEGES ON `digikam`.* TO 'digiuser'@'localhost'; >CREATE DATABASE digikam; > >The conversion code was failing to catch SQL errors in the migration >process. I corrected this by returning an error if a query fails but it is >not clear if this is the intended behaviour and it maybe that this will >uncover other issues both in SQLite and MySQL. > >The Albums table has an icon field that causes a circular reference to >the Images table. This needs special handling both during table deletion >and data migration. I put a hack in place for the table deletion case but >did not address the migration where images will initially need creating >without the icon being set and then the icon values need copying over >once the Images have been migrated. > >The migration will fail if any orphan records exist that do not reference an >existing AlbumRoot, Album, Image or Tag. Normally that would be due to >some previous uncaught error due to failed creation or partial deletion. > >I did notice that in my newly created SQLite database with only a few test >images the Tags "icon" field was populated with a zero instead of a null. >This causes the migration to fail. I worked around this by executing the >following against the SQLite database. > >update Tags set icon = null where icon = 0; > >The thumbnail and face schema have not been fully examined/converted yet. >The triggers have been disabled to allow migration testing. > >Similarly I have not looked at database upgrade from the previous version >of the MySQL schema. There is at least one user (myself) who has been >using MySQL so having a proper upgrade to add the referential integrity >would seem to be needed in a final solution. > > >Richard Mortimer (5): > Ensure that an error is returned if the query fails. > Delete tables in reverse creation order. > Ensure that data is migrated in line with foreign key dependencies. > Remove trigger dependency for MySQL. > Temporary workaround to break MySQL references loop with the Albums > icon entry. > > data/database/dbconfig.xml.cmake.in | 82 +++++++++++++++--------------- > libs/database/coredb/coredbcopymanager.cpp | 9 ++-- > libs/database/engine/dbenginebackend.cpp | 2 + > 3 files changed, 48 insertions(+), 45 deletions(-) > >-- >2.5.0 >
Git commit e9cad0b63d2d677d58ab55e8208af591f9cd1298 by Gilles Caulier. Committed on 24/11/2015 at 12:46. Pushed by cgilles into branch 'master'. apply patch #95688 to delete tables in reverse creation order. M +1 -1 libs/database/coredb/coredbcopymanager.cpp http://commits.kde.org/digikam/e9cad0b63d2d677d58ab55e8208af591f9cd1298
Richard, About patch 4/5, i need to test. A lots of code SQL has changed. About patch 5/5, if you have a better solution to patch SQL statements, let's me hear. Gilles
About 5/5 : why CreateFaceTriggers is commented now ? About database init, before we use : CREATE DATABASE digikam; GRANT ALL PRIVILEGES ON digikam.* TO 'root'@'localhost' IDENTIFIED BY 'password'; FLUSH PRIVILEGES; now, you use : GRANT USAGE ON *.* TO 'digiuser'@'localhost' IDENTIFIED BY 'abc123'; GRANT ALL PRIVILEGES ON `digikam`.* TO 'digiuser'@'localhost'; CREATE DATABASE digikam; => You create database at end. Why ? => you grant usage to dedicated user. Why ? => you grant privilege to this dedicated user too. Why ? => you do not flush privilege. Why ? Is you rules are to create a database only usable by a dedicated user, and not root ? This will permit to share the same database with more than one users at the same time ? Gilles Caulier
If the schema is updated, the schema version must be patched in *schemaupdater.cpp classes. New rules to update schema with relevant DB action must be create to be able to migrate old schema to new one. Gilles Caulier
CreateFaceTriggers is commented because it is still a TODO change to replace it with references. I just commented it to stop any chance of an error with the reduced privileges whilst testing. I will replace this when I produce another version of that patch. => You create database at end. Why ? It will work at the start or end. It is just habit that I create the database after setting up permissions. => you grant usage to dedicated user. Why ? The usage line sets up the password for the dedicated user. The password is not tied to a specific database so it is clearer to set that up separately from giving access to a database. Note that "USAGE" infers no privileges to a user it just provides a convenient way to change user account settings. See http://dev.mysql.com/doc/refman/5.5/en/privileges-provided.html#priv_usage => you grant privilege to this dedicated user too. Why ? The second grant gives access to the specific database for that user. That is done at database level so it give all database level privileges (create database, table etc.) without giving database server administration level privileges. => you do not flush privilege. Why ? I do not think that flush is needed if you use the GRANT statement method to setup privileges. But it does not hurt to include it. See http://dev.mysql.com/doc/refman/5.5/en/privilege-changes.html Use of a dedicated user and not root is for security reasons. The root user tends to have full database administration privileges and that is not a good thing to encourage. If you want to give multiple users access to the same database just issue the same grant commands for the additional users. You could also use a more tightly controlled set of privileges for these different users.
About patch 5/5 : it's Sqlite or/and Mysql relevant fix ? Don't forget that SQlite schema can be considerate as stable. So take a care about change here. An update version must be created with a db action dedicated to migrate to new schema.
You explanation in comment #14 are fine for me. It's definitively the good way to go. About Mysql, as the support have been always considerated as experimental, if we change a lots the DB schema, the best way will be to set older one as obsolete and incompatible with new one. Everywhere, the users must be warned about the major changes introduced to the new schema and no migration rules cannot be written easily. Typically, in this case, the user must use XMP sidecar solution over collection for ex to save DK metadata, and create a new mysql DB with a fresh parsing. This will reduce the complexity in SQL statements rules. What do you think about ? Gilles Caulier
Created attachment 95731 [details] Updated trigger removal patch for MySQL databases
I have not had chance to look at MySQL upgrade procedure yet - although given the changes made so far it should just be adding foreign keys so assuming that the data is not broken due to orphaned records then that should be OK. Similarly I have not had chance to look at the tags issue on MySQL yet. A couple of other things I have noticed: 1 - Each of the image, thumbnails and face recognition databases all have a table called Settings. I can see this causing trouble with all three merged into a single database. At a very minimum the migration needs to take care to track this and migrating back to separate databases would be even harder. Is it going to be feasible to rename the thumbnails and face recognition tables to something like SettingsThumb and SettingsFace or would that cause too much trouble with SQLite support? 2 - Currently migration does not seem to handle the faces and thumbnails databases. Presumably this is planned but just not done yet. 3 - the logic surrounding m_isStopProcessing in coredbcopymanager.cpp looks to be wrong in places. The loops do a OR operation, e.g. "m_isStopProcessing || i >=0", but not all of them test m_isStopProcessing to terminate the migration if that flag is set. Also using that way to terminate the loop the migration the code must be careful not to access the loop counter "i" before terminating because it could cause an overflow of the tables array if the termination is received during the processing of the last table being iterated.
(In reply to Richard Mortimer from comment #17) > Created attachment 95731 [details] > Updated trigger removal patch for MySQL databases I forgot to note that the current patch still does not handle the deferred copy of the icon value in the Albums table.
>1 - Each of the image, thumbnails and face recognition databases all have a table called Settings. >I can see this causing trouble with all three merged into a single database. Which trouble exactly ? I already noticed some problem and i fixed this with face recognition DB integration. >At a very minimum the migration needs to take care to track this and migrating back to separate >databases would be even harder. Is it going to be feasible to rename the thumbnails and face >recognition tables to something like SettingsThumb and SettingsFace or would that cause too >much trouble with SQLite support? I thinking about to do the same. I think it's safe if Settings table differ than SQlite for MySql. 2/ => ok. 3/ => the logic is good. turning on the bool value will stop copy processing. But in fact an new method need to be add in this class to be able to change this private bool member from outside (as for ex, when Cancel button is pressed). Granularity of bool check to cancel copy must be decreased. typically checking bool outside the loop will prevent to break partial database copy in the middle section of data. In all case a garbage collector must be implemented to cleanup previous copy while migration. Perhaps to cleanup target database can be enough.
(In reply to caulier.gilles from comment #20) > >1 - Each of the image, thumbnails and face recognition databases all have a table called Settings. >I can see this causing trouble with all three merged into a single database. > > Which trouble exactly ? I already noticed some problem and i fixed this with > face recognition DB integration. > Sorry. That was a poor description from me. I was not referring to a specific problem. I meant that it is likely to cause confusion/problems in the future. > 3/ => the logic is good. turning on the bool value will stop copy > processing. But in fact an new method need to be add in this class to be > able to change this private bool member from outside (as for ex, when Cancel > button is pressed). As I said the current code code is broken in at least 2 places. They are easy to fix but I just wanted to record it before I forgot. The specific issues I can see are: If set during the "Delete all tables" loop the loop will never terminate (m_isStopProcessing || i>=0 will always be true) and will result in a crash once "i" becomes negative and underflows the tables array. Similarly if set during the copying the last iteration of the copy tables loop then "i" will become greater than the length of tables and will result in a crash during emit stepStarted because it tries to access tables[i]. > > Granularity of bool check to cancel copy must be decreased. typically > checking bool outside the loop will prevent to break partial database copy > in the middle section of data. agreed. > > In all case a garbage collector must be implemented to cleanup previous copy > while migration. Perhaps to cleanup target database can be enough. agreed. The user must expect that the target database has been changed and a cleanup should be enough.
Created attachment 95746 [details] Defer migration of Albums.icon until after the Images table has been migrated
Created attachment 95747 [details] Fix array under/overflow when m_isStopProcessing is set to true
Created attachment 95755 [details] Move Thumbnail database Settings to ThumbSettings for MySQL This allows the thumbnail and main image databases to be contained within the same MySQL database. Also ensure that thumbnail MySQL tables use the InnoDB storage engine and not the older MyISAM engine. I had to change the longtext fields to varchar(767) to stop MySQL complaining on upgrade. But also note that those same fields have a 255 character long unique index so in practice anything over 255 characters is likely to cause problems.
Comment on attachment 95747 [details] Fix array under/overflow when m_isStopProcessing is set to true >From 61762e8ed5083c791d553ddc43f278c7e08b54eb Mon Sep 17 00:00:00 2001 >From: Richard Mortimer <richm@oldelvet.org.uk> >Date: Wed, 25 Nov 2015 21:10:23 +0000 >Subject: [PATCH 3/3] Ensure that m_isStopProcessing does not cause array > overshoots > >--- > libs/database/coredb/coredbcopymanager.cpp | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > >diff --git a/libs/database/coredb/coredbcopymanager.cpp b/libs/database/coredb/coredbcopymanager.cpp >index e2f3115..e3cbf09 100644 >--- a/libs/database/coredb/coredbcopymanager.cpp >+++ b/libs/database/coredb/coredbcopymanager.cpp >@@ -126,7 +126,8 @@ void CoreDbCopyManager::copyDatabases(const DbEngineParameters& fromDBParameters > > for (int i=(tablesSize - 1); m_isStopProcessing || i >= 0; --i) > { >- if (toDBbackend.execDirectSql(QString::fromUtf8("DROP TABLE IF EXISTS %1;").arg(tables[i])) != BdEngineBackend::NoErrors) >+ if ( m_isStopProcessing || >+ toDBbackend.execDirectSql(QString::fromUtf8("DROP TABLE IF EXISTS %1;").arg(tables[i])) != BdEngineBackend::NoErrors) > { > emit finished(CoreDbCopyManager::failed, i18n("Error while scrubbing the target database.")); > fromDBbackend.close(); >@@ -135,7 +136,8 @@ void CoreDbCopyManager::copyDatabases(const DbEngineParameters& fromDBParameters > } > } > >- if (toDBbackend.execDirectSql(QString::fromUtf8("DROP TABLE IF EXISTS Settings;")) != BdEngineBackend::NoErrors) >+ if ( m_isStopProcessing || >+ toDBbackend.execDirectSql(QString::fromUtf8("DROP TABLE IF EXISTS Settings;")) != BdEngineBackend::NoErrors) > { > emit finished(CoreDbCopyManager::failed, i18n("Error while scrubbing the target database.")); > fromDBbackend.close(); >@@ -162,7 +164,9 @@ void CoreDbCopyManager::copyDatabases(const DbEngineParameters& fromDBParameters > > for (int i=0; m_isStopProcessing || i < tablesSize; ++i) > { >- emit stepStarted(i18n(QString::fromUtf8("Copy %1...").arg(tables[i]).toLatin1().constData())); >+ if (i < tablesSize) { >+ emit stepStarted(i18n(QString::fromUtf8("Copy %1...").arg(tables[i]).toLatin1().constData())); >+ } > > // Now perform the copy action > >-- >2.5.0 >
Git commit e380a053a5273130e65b2d13976261cd9ca1a90a by Gilles Caulier. Committed on 26/11/2015 at 21:50. Pushed by cgilles into branch 'master'. apply patch #95747 to pezvznt array overshoots in loop when cancel is applied. M +13 -8 libs/database/coredb/coredbcopymanager.cpp http://commits.kde.org/digikam/e380a053a5273130e65b2d13976261cd9ca1a90a
Richard, I start to test step by step all pending patches. > For patch : Remove trigger dependency for MySQL. ==> the patch do not touch the dbsettingswidget.cpp code where SQL requirements is given to previously create the databases in remote Mysql server by the system admin. ==> This give a lots of error on the console when i use Mysql internal server (i don't yet tested Mysql remote server yet). Note that Mysql Internal server use "digikam" database name for all database (Core, Thumbs, Face). Look errors below : digikam.dbengine: Failure executing query: "UPDATE Tags SET icon=? WHERE id=?;" Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update a child row: a foreign key constraint fails (`digikam`.`Tags`, CONSTRAINT `Tags_Images` FOREIGN KEY (`icon`) REFERENCES `Images` (`id`) ON DELETE SET NULL ON UPDATE CASCADE)" 1452 2 Bound values: (QVariant(qlonglong, 0), QVariant(int, 1)) digikam.dbengine: Failure executing query: "UPDATE Tags SET icon=? WHERE id=?;" Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update a child row: a foreign key constraint fails (`digikam`.`Tags`, CONSTRAINT `Tags_Images` FOREIGN KEY (`icon`) REFERENCES `Images` (`id`) ON DELETE SET NULL ON UPDATE CASCADE)" 1452 2 Bound values: (QVariant(qlonglong, 0), QVariant(int, 2)) digikam.dbengine: Failure executing query: "UPDATE Tags SET icon=? WHERE id=?;" Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update a child row: a foreign key constraint fails (`digikam`.`Tags`, CONSTRAINT `Tags_Images` FOREIGN KEY (`icon`) REFERENCES `Images` (`id`) ON DELETE SET NULL ON UPDATE CASCADE)" 1452 2 Bound values: (QVariant(qlonglong, 0), QVariant(int, 3)) digikam.dbengine: Failure executing query: "UPDATE Tags SET icon=? WHERE id=?;" Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update a child row: a foreign key constraint fails (`digikam`.`Tags`, CONSTRAINT `Tags_Images` FOREIGN KEY (`icon`) REFERENCES `Images` (`id`) ON DELETE SET NULL ON UPDATE CASCADE)" 1452 2 Bound values: (QVariant(qlonglong, 0), QVariant(int, 4)) digikam.dbengine: Failure executing query: "UPDATE Tags SET icon=? WHERE id=?;" Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update a child row: a foreign key constraint fails (`digikam`.`Tags`, CONSTRAINT `Tags_Images` FOREIGN KEY (`icon`) REFERENCES `Images` (`id`) ON DELETE SET NULL ON UPDATE CASCADE)" 1452 2 Bound values: (QVariant(qlonglong, 0), QVariant(int, 5)) ... and more... > For patch : Defer migration of Albums.icon until after the Images table has been migrated ==> sound like it need the previous patch to be applied. the patch is rejected. Did you confirm ? > For patch : Move Thumbnail database Settings to ThumbSettings for MySQL ==> We need the same solution for Face database which also create a Settings table. ==> sound like it need the previous patch to be applied. the patch is rejected. Did you confirm ? ==> After renaming Thumb and Face DB Settings tables, this will fix bug #331628 ? At least, as you specify InnoDB database engine, TokuDB engine will be not possible to use instead. Right ? Gilles
(In reply to caulier.gilles from comment #27) > Richard, > > I start to test step by step all pending patches. > > > For patch : Remove trigger dependency for MySQL. > ==> the patch do not touch the dbsettingswidget.cpp code where SQL > requirements is given to previously create the databases in remote Mysql > server by the system admin. OK. I haven't looked at internal MySQL yet. I've got quite a few MySQL database servers hanging around so it is easier for me to test and debug there in the early stages. > ==> This give a lots of error on the console when i use Mysql internal > server (i don't yet tested Mysql remote server yet). Note that Mysql > Internal server use "digikam" database name for all database (Core, Thumbs, > Face). Look errors below : Did those errors cause the migration to fail? Or did it complete with just the complaints on the console? > > digikam.dbengine: Failure executing query: > "UPDATE Tags SET icon=? WHERE id=?;" > Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update > a child row: a foreign key constraint fails (`digikam`.`Tags`, CONSTRAINT > `Tags_Images` FOREIGN KEY (`icon`) REFERENCES `Images` (`id`) ON DELETE SET > NULL ON UPDATE CASCADE)" 1452 2 > Bound values: (QVariant(qlonglong, 0), QVariant(int, 1)) Those errors look like there are some Tags that have icons setup to point at non-existant images. The examples you gave all have the first bound value as zero which is typically a non-existant icon (instead of null). I suspect most/all are set to zero instead of null. We should be able to filter out those definitely bad ones. What are your thoughts on filtering out data that is broken because the Album, Image, Tag it refers to does not exist in the database? I tend to think that the data was not accessible to users in the original database so it should be safe to not transfer it. > > For patch : Defer migration of Albums.icon until after the Images table has been migrated > > ==> sound like it need the previous patch to be applied. the patch is > rejected. Did you confirm ? Ah sorry. I rebased my patches based on what you had already committed to the public git and squashed updates on a single topic into a single patch. I thought it best to make the patch in bugzilla be clear about that whole changes for a particular reason. > > > For patch : Move Thumbnail database Settings to ThumbSettings for MySQL > > ==> We need the same solution for Face database which also create a Settings > table. Yes. I did not get around to that yet. Am I correct in that we can assume no need to provide an upgrade here because there are no existing MySQL users with face settings yet. > ==> sound like it need the previous patch to be applied. the patch is > rejected. Did you confirm ? Yes. It does require the previous patch. > ==> After renaming Thumb and Face DB Settings tables, this will fix bug > #331628 ? At least, as you specify InnoDB database engine, TokuDB engine > will be not possible to use instead. Right ? Hmmm. A difficult problem. Really we just want to NOT use MyISAM because it does not support referential integrity. TokuDB should work and it would be possible to manually change all the tables after upgrade. If automatic support for a particular backend was required then I think it would have to be specified on the setup/migration dialog and then manually specified during the migration. Richard
>Did those errors cause the migration to fail? Or did it complete with just the complaints on the console? ==> No migration here. As now first run assistant support Mysql as well, we can satrt to init a DB with Mysql as well. So, as schema is changed a lots, i always start tests with a fresh DB. >TokuDB should work and it would be possible to manually change all the tables after upgrade. >If automatic support for a particular backend was required then I think it would have to be >specified on the setup/migration dialog and then manually specified during the migration. ==> in fact i don't care about TokuDB engine. If we specify in table creation that Immno engine must be used as well, because we have tested with it only, it's fine for me. The question is more to kill this bug with right comment, as TokuDB engine is not supported and digiKam use Inmmo engine. The goal is not to introduce complexity, it's already enough complex. As Mysql is able to support tables with different engines at the same time (fix me if i'm wrong), digiKam must be usable as well. Right ? Gilles
(In reply to caulier.gilles from comment #29) > >Did those errors cause the migration to fail? Or did it complete with just the complaints on the console? > > ==> No migration here. As now first run assistant support Mysql as well, we > can satrt to init a DB with Mysql as well. So, as schema is changed a lots, > i always start tests with a fresh DB. OK. I see the error now and what causes it. The AlbumDB::addTag routine in albumdb.cpp gets passed an iconID value of zero. That presumably means "no icon" but with referential integrity enabled the update fails because there is no image zero in the database. An easy solution would be to filter out any iconID = 0 values and set the icon to null in those cases. It is not 100% safe because the database could decide to use imageID = 0 for a real image but it would work in most cases. I can see similar usages in setTagIcon plus the equivalent functions for Album icons. > > >TokuDB should work and it would be possible to manually change all the tables after upgrade. > >If automatic support for a particular backend was required then I think it would have to be >specified on the setup/migration dialog and then manually specified during the migration. > > ==> in fact i don't care about TokuDB engine. If we specify in table > creation that Immno engine must be used as well, because we have tested with > it only, it's fine for me. > > The question is more to kill this bug with right comment, as TokuDB engine > is not supported and digiKam use Inmmo engine. The goal is not to introduce > complexity, it's already enough complex. I think the correct answer is that we test and use InnoDB. There is nothing to stop someone changing the engine to TokuDB and it should work (no guarantees) but the engine may get changed back to InnoDB if/when digikam is upgraded. > > As Mysql is able to support tables with different engines at the same time > (fix me if i'm wrong), digiKam must be usable as well. Right ? Yes. I have produced a patch to use a "FaceSettings" table for faces. Would you like me to produce clean patches against master to change both Face and Thumbnail databases to the new settings. I think those two are less likely to break than the main database code because they do not use null columns in quite the same way as the album/tags icons tables. Richard
Richard, With your Core DB schema update, this bug wil be fixed ? https://bugs.kde.org/show_bug.cgi?id=286492 Gilles
Richard, Same question than comment #31 abou this file : https://bugs.kde.org/show_bug.cgi?id=350574 Gilles
Richard, What's about to use Foreign Keys in DB schema ? https://bugs.kde.org/show_bug.cgi?id=237878 It's safe since Mysql and MariaDb support it ? Gilles
Richard, I think file https://bugs.kde.org/show_bug.cgi?id=281838 ... is fixed since a while. Can you confirm ? Gilles
Created attachment 95823 [details] Standalone MySQL Thumbnail settings to ThumbSettings table Move Thumbnail database Settings to ThumbSettings for MySQL This allows the thumbnail and main image databases to be contained within the same database. Also ensure that thumbnail MySQL tables use the InnoDB storage engine and not the older MyISAM engine.
Created attachment 95824 [details] Standalone MySQL face recognition settings to FaceSettings table Update face recognition MySQL support. Use FaceSettings table to allow shared database in MySQL. Use InnoDB engine to ensure that foreign key relationships are enforced. Use foreign key between Identities and IdentityAttributes.
Created attachment 95825 [details] Standalone MySQL Thumbnail settings to ThumbSettings table
Thanks Richard, I will review your patches tomorow or wenesday... Gilles
Gilles, 1) I did not yet get chance to rebase the other two patches to separate those from the face and thumbnail patches. I will do so within the next day. 2) I started to look at the Tags and Albums icon code. The entire code seems to assume that an icon value of zero means no icon. This is not good for referential integrity. Ideally all usages of qlonglong for icon value should be replaced with something like QVariant that allows a null value too. I think this is too big a change at the moment so my suggestion is that the database storage code is modified to hardcode 0 as being equivalent to null in the database. I did a quick test on both MySQL and SQLite and both work fine with that assumption because when reading from the database a null icon is turned into a zero when reading a longlong from QVariant that is null. Doing a full QVariant conversion for icon fields might be something that could be included in a GSoC project for PostgreSQL support.
Git commit 04baf90347ae3365378333f6ae4f84c1dc39c73d by Gilles Caulier. Committed on 01/12/2015 at 21:31. Pushed by cgilles into branch 'master'. apply patch #95824 to : Use FaceSettings table to allow shared database in MySQL. Use InnoDB engine to ensure that foreign key relationships are enforced. Use foreign key between Identities and IdentityAttributes. M +31 -16 data/database/dbconfig.xml.cmake.in M +9 -6 libs/facesengine/facedb/facedb.cpp M +1 -1 libs/facesengine/facedb/facedb.h http://commits.kde.org/digikam/04baf90347ae3365378333f6ae4f84c1dc39c73d
Git commit 9b543e6a84a15af6d29f70c39a4964992f42e7cf by Gilles Caulier. Committed on 01/12/2015 at 21:34. Pushed by cgilles into branch 'master'. Apply patch #95825 to : Allows the thumbnail and main image databases to be contained within the same database. Also ensure that thumbnail MySQL tables use the InnoDB storage engine and not the older MyISAM engine. M +66 -10 data/database/dbconfig.xml.cmake.in M +27 -4 libs/database/thumbsdb/thumbsdb.cpp M +1 -0 libs/database/thumbsdb/thumbsdb.h M +23 -1 libs/database/thumbsdb/thumbsdbchemaupdater.cpp M +1 -0 libs/database/thumbsdb/thumbsdbchemaupdater.h http://commits.kde.org/digikam/9b543e6a84a15af6d29f70c39a4964992f42e7cf
Created attachment 95860 [details] Store empty icon (image 0) as a NULL value Tested on both SQLite and MySQL databases. Album and tag icons can be set and cleared.
Created attachment 95861 [details] Remove coredb trigger dependency for MySQL. This is still a work in progress. Known issues: 1 - Tags broken on MySQL. 2 - Existing "zero" value icons break database migration. 3 - No support for upgrade of existing coredb contents on MySQL. 4 - On migration from SQLite to MySQL I noticed breakages in filenames on child albums thumbnails. Works fine on a fresh MySQL database setup. Needs more investigation.
Git commit bca001b16dd6aba437aece1bc09e9400ec300d1d by Gilles Caulier. Committed on 02/12/2015 at 10:50. Pushed by cgilles into branch 'master'. Apply patch #95860 to store empty icon (image 0) as a NULL value with SQlite and Mysql Databases digiKam treats image zero as a "not set" icon value. This breaks referential integrity in the database because image zero does not exist. Explicitly convert a value of zero into NULL when stored in the database. Ideally digiKam should use an explicit null (in QVariant?) placeholder for the no-icon case but currently no database stores an image zero by default so the existing practice works. Note that when reading from the database the toLongLong method of QVariant returns a zero when called against a null value so this means that no changes are required when reading null values in from the database. M +26 -5 libs/database/coredb/coredb.cpp http://commits.kde.org/digikam/bca001b16dd6aba437aece1bc09e9400ec300d1d
Git commit 801f835f246b2b2a38a886920056c149dd6894c0 by Gilles Caulier. Committed on 02/12/2015 at 11:04. Pushed by cgilles into branch 'master'. apply patch #95861 to remove trigger dependency for MySQL. Add full referential integrity to AlbumRoots, Albums, Images and Tags tables entries. Provide the equivalent behaviour to the triggers using ON DELETE and ON UPDATE in FOREIGN KEY references. Use dbaction to perform database specific preparation for migration. The MySQL schema has a circular dependency and this must be removed before any leftover contents of the database are removed prior to the migration. No actions are required for SQLite Referential integrity in MySQL does not allow the Albums.icon field to be set prior to the Images table being populated. Use a fake AlbumsExtra table to copy over the icons data. Note we use the same procedure for both SQLite and MySQL to ensure that migration is possible between any combination of databases. M +63 -51 data/database/dbconfig.xml.cmake.in M +12 -0 libs/database/coredb/coredbcopymanager.cpp http://commits.kde.org/digikam/801f835f246b2b2a38a886920056c149dd6894c0
Richard, Did you plan to specific ImnoDb database type with Mysql for Core database creation action, as with Thumbs and Face databases ? If yes, this will permit to close bug #331628 as well... Also, in Mysql, The Core database settings table is named "Settings", where for Thumbs database is now named ThumbSettings and for Face database FaceSettings. Why not to rename Settings table as CoreSettings to be homogeneous everywhere ? Gilles
(In reply to caulier.gilles from comment #46) > Richard, > > Did you plan to specific ImnoDb database type with Mysql for Core database > creation action, as with Thumbs and Face databases ? Yes. That seems to have got lost somewhere in all the rebasing. > > If yes, this will permit to close bug #331628 as well... > > Also, in Mysql, The Core database settings table is named "Settings", where > for Thumbs database is now named ThumbSettings and for Face database > FaceSettings. Why not to rename Settings table as CoreSettings to be > homogeneous everywhere ? Will look to implement that when the coredb MySQL schema upgrade scripts are written. Richard
Created attachment 95865 [details] MySQL database permissions/setup guidance streamlining. Make it clear that the password is specific to the account/hostname combination and not the database.
Created attachment 95866 [details] Specify InnoDB engine on setup for coredb tables.
Created attachment 95867 [details] Do not migrate "zero" icon values for Albums and Tags MySQL referential integrity does not allow to refer to a non-existant placeholder image. Use a null value instead.
Git commit 5c1416f17b230a598b8938ee3a18bb31b65303d0 by Gilles Caulier. Committed on 02/12/2015 at 21:21. Pushed by cgilles into branch 'master'. Apply patch #95865 to rewrite MySQL database creation and setup guidance. The MySQL password is tied to the account not the database. M +10 -8 libs/database/utils/dbsettingswidget.cpp http://commits.kde.org/digikam/5c1416f17b230a598b8938ee3a18bb31b65303d0
Git commit 1f1855693efd9b01a7149853eb59596b1831b5f4 by Gilles Caulier. Committed on 02/12/2015 at 21:23. Pushed by cgilles into branch 'master'. Apply patch #95867 to filter out existing zero (no icon set) icon values during migration. This is required to enforce referential integrity during migration to a MySQL database. M +4 -4 data/database/dbconfig.xml.cmake.in http://commits.kde.org/digikam/1f1855693efd9b01a7149853eb59596b1831b5f4
Git commit 3eefa0b7493ee7abb72d6532e618a3769e181e2f by Gilles Caulier. Committed on 02/12/2015 at 21:28. Pushed by cgilles into branch 'master'. Apply patch #95866 to force use of InnoDB engine with all Mysql database table. Related: bug 281838, bug 331628 FIXED-IN: 5.0.0 M +3 -1 NEWS M +40 -21 data/database/dbconfig.xml.cmake.in http://commits.kde.org/digikam/3eefa0b7493ee7abb72d6532e618a3769e181e2f
Richard, The Mysql Database schema must take a care at least about these bugs : 1/ optimisations with foreign keys . I see that you start to do it with FaceDb. I don't know if you plan for other Db. https://bugs.kde.org/show_bug.cgi?id=237878 2/ Migration dysfunctions : i don't know if they still valid or not. https://bugs.kde.org/show_bug.cgi?id=286492 https://bugs.kde.org/show_bug.cgi?id=350574 https://bugs.kde.org/show_bug.cgi?id=316690 https://bugs.kde.org/show_bug.cgi?id=325655 3/ Mysql and case-insensitive file path : https://bugs.kde.org/show_bug.cgi?id=268204 https://bugs.kde.org/show_bug.cgi?id=355893 4/ Items filter which doesn't work for video type-mime : https://bugs.kde.org/show_bug.cgi?id=327957 They are other entries to check, but we will take a look later... Gilles
Created attachment 95928 [details] Experiment with TagsTree tables. Build a TagsTree2 file that mimics the existing behaviour of the MySQL Tags table and use that for all tags tree queries. The advantage of a separate table is that it can be rebuilt without destroying or changing main Tags table. This is experimental and it would be good if people can exercise the relevant tags behaviour against their database to ensure that a wide variety of queries have been tested. If all goes well the intention would be to rename all uses of TagsTree2 to TagsTree prior to final committal.
Richard, How to test quickly your new patch #95928 exactly ? Gilles
(In reply to caulier.gilles from comment #56) > How to test quickly your new patch #95928 exactly ? General testing of the tags functionality with a MySQL database. I'm not 100% sure how to test it all but a lot if it is in the searching by tag hierarchy and viewing specific tags. I did test the queries manually but could not excercise the the C++ code to ensure that the relevant SQL generators were running correctly.
Richard To process C++ test with database, we can use QTest API. http://doc.qt.io/qt-5/qtest-overview.html Currently, we have a core/tests/ code to process this kind of check over a small SQlite database and also MySQL. It located in albummodel and database sub-dirs. https://projects.kde.org/projects/extragear/graphics/digikam/repository/revisions/master/show/tests/ This code will generate a small CLI application which will process usual tests with database schema content. The code init a DK core instance, create a database with few image collection from data sub folder and try to run all basic operations in database. The QTest API permit to check the results step by step. And end we can have a resume of tests processed with success or not. This kind of tests application is started automatically by the KDE Jenkins server, after a complete core compilation, started after each commit. https://build.kde.org/job/digikam%20master%20kf5-qt5/ To have all test code compiled in digiKam, use the cmake flag -DBUILD_TESTING=ON while configuration of digiKam repository. This will enable tests sub-dir compilation. the tests CLI tools will be compiled in build directory and can be started as well from a console : [gilles@localhost database]$ ./databasefieldstest ********* Start testing of DatabaseFieldsTest ********* Config: Using QtTest library 5.5.1, Qt 5.5.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 5.2.1 20151020) PASS : DatabaseFieldsTest::initTestCase() PASS : DatabaseFieldsTest::testMinSizeType() PASS : DatabaseFieldsTest::testIterators() PASS : DatabaseFieldsTest::testMetaInfo() PASS : DatabaseFieldsTest::testIteratorsSetOnly() PASS : DatabaseFieldsTest::testSet() PASS : DatabaseFieldsTest::testSetHashAddSets() PASS : DatabaseFieldsTest::testHashRemoveAll() PASS : DatabaseFieldsTest::cleanupTestCase() Totals: 9 passed, 0 failed, 0 skipped, 0 blacklisted ********* Finished testing of DatabaseFieldsTest ********* [gilles@localhost database]$ pwd /home/gilles/Devel/5.x/build/core/tests/database [gilles@localhost database]$ There are 3 tools : - testdatabase : it reproduce the basis database core init and shutdown. It used with valgrind to check if core engine has memory leak. You can forget this one for the moment. - databasefieldstest : this one is check if the field is DB are processed with core engine API properly. This one do not play with tags tree. - albummodeltest : create another small database (SQlite only for the moment) to check DB album properties in all conditions. So, if you need a QTest code to check tags tree in DB, i recommend to create a new small CLI tool based on 2 last one previously described. It's not very well complicated. init and populate a small DB (Sqlite/Mysql internal) with a small collection of image including tags in metadata. This will load automatically the tags tree in DB with DB scan code. After that you can try to change tags tree as you want in code and use QTest to check if the changes results is right or not. Gilles
Richard, i tested your patch #95928, and i can see a lots of dupplicates tags created with the same name when i create a new database (mysql internal). My collection of image : digikam version 5.0.0-beta3 Images: JP2: 10 JPG: 1549 PGF: 7 PNG: 141 PPM: 2 RAW-ARW: 13 RAW-CR2: 45 RAW-CRW: 5 RAW-DCR: 2 RAW-DNG: 18 RAW-MRW: 18 RAW-NEF: 19 RAW-ORF: 4 RAW-PEF: 7 RAW-RAF: 10 RAW-RAW: 1 RAW-RWL: 1 RAW-X3F: 2 TIFF: 28 total: 1882 : Videos: MOV: 6 total: 6 : Total Items: 1888 Albums: 164 Tags: 90 : Database backend: QMYSQL Database internal server: Yes Database internal server Path: /mnt/data While importing collection in DB, i can see also these warning, about versionning metadata imported from XMP : digikam.dimg: "/mnt/data/TEST_IMAGES/splashs/showfoto-splash.png" : PNG file identified digikam.database: Adding new item "/mnt/data/TEST_IMAGES/splashs/showfoto-splash.png" digikam.metaengine: DateTime => Exif.Photo.DateTimeOriginal => QDateTime(2010-12-29 00:00:00.000 CET Qt::TimeSpec(LocalTime)) digikam.metaengine: DateTime => Exif.Photo.DateTimeOriginal => QDateTime(2010-12-29 00:00:00.000 CET Qt::TimeSpec(LocalTime)) digikam.metaengine: "/mnt/data/TEST_IMAGES/splashs/showfoto-splash.png" ==> Read Iptc Keywords: () digikam.database: Pick Label found : 0 digikam.database: Cannot find Pick Label Tag for : 0 digikam.database: Color Label found : 0 digikam.database: Cannot find Color Label Tag for : 0 digikam.metaengine: Loading image history "" digikam.database: Scanning took 3 ms digikam.database: Finishing took 1 ms digikam.database: Folder does not exist or is not readable: "/mnt/data/lost+found" digikam.database: Attempt to create tag properties for tag id 0 digikam.database: items to tag (2, 178, 184, 1199, 1383, 1450, 1543, 1563, 1567, 1568, 1806, 1814, 1837, 1838, 1843, 1844, 1846, 1849, 1850, 1854, 1858, 1859, 1860, 1861, 1864, 1865, 1869) digikam.database: Broken history: Same file referred by different entries. Refusing to add a loop. digikam.database: Graph with 1 vertices: "Unconnected: { Id: 2 UUID: e8ff0e... }" digikam.database: Graph with 2 vertices: "{ Ids: () UUID: a7150e... } -> { Id: 178 UUID: a00753... }" digikam.database: Image 178 type QFlags(0x8) digikam.database: Graph with 1 vertices: "Unconnected: { Id: 184 }" digikam.database: Graph with 2 vertices: "{ Ids: () UUID: fcac8f... } -> { Id: 1199 UUID: c9fecb... }" digikam.database: Image 1199 type QFlags(0x8) digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1383 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1450 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1543 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1563 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1567 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1568 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1806 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1814 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1837 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1838 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1843 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1844 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1846 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1849 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1850 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1854 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1858 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1859 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1860 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1861 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1864 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1865 }" digikam.database: Graph with 1 vertices: "Unconnected: { Id: 1869 }" digikam.database: Complete scan took: 31320 msecs. Gilles
Gilles, Many thanks for the information and the testing feedback. I have not had much time to look into this yet. A quick look at the schema does indicate that there is a missing UNIQUE clause in the Tags table definition for MySQL. I do not think that is the full solution but it will certainly stop duplicate entries reaching the database.
Richard, I started to write a test cases implementation for Mysql and tree-view. I need to fix some point of Mysql database settings, especially with internal database server that i will use for that. Code is here but not completed and compiled for the moment : https://projects.kde.org/projects/extragear/graphics/digikam/repository/revisions/master/entry/tests/database/databasetagstest.cpp I will continue this week end and during this Christmas holidays... Gilles Caulier
Hello, currently I face the issue of not being able to completely delete directories. Directories are deleted on disk, but not deleted from the database, i.e., obsolete ghost directories remain in Digikam's album tree. Seems to be related to failing MySQL queries: "UPDATE Albums SET albumRoot=0, relativePath=? WHERE id=?;" Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update a child row: a foreign key constraint fails (`digikam_core`.`Albums`, CONSTRAINT `Albums_AlbumRoots` FOREIGN KEY (`albumRoot`) REFERENCES `AlbumRoots` (`id`) ON DELETE CASCADE ON UPDATE CASCADE)" 1452 2 Bound values: (QVariant(QString, "1-/test"), QVariant(int, 2)) Log: https://bpaste.net/show/120359f6c1f2 Version: 5.0.0-beta3 <-> 5.0.0-beta4 Commit c75e88ddb078cc2aef3ea3427b934fe18fda906f Sat, 20 Feb 2016 08:35:35 +0000 (09:34 +0059) Setup / Reproduce: 1) Digikam started with three empty MySQL databases, i.e., core, faces, and thumbnails 2) Add test collection 3) Add test album / directory to collection 4) Delete test album -> failure, i.e., album stays in Digikam album tree, deleted on disk, MySQL error in log 5) Delete test (ghost) album again -> no change Maybe this helps to fix it. As workaround, recreating a ghost album as directory manually at disk makes it reusable in Digikam. I run Digikam with MySQL databases and would be glad to help testing. Regards Felix
Hello Richard. I'm Swati, currently working on digiKam MySQL database support project. I read this open bug for MySQL Schema improvements and thought of a plausible solution to handle the tags (rename, move or delete) An image can have many tags and a tag can belong to multiple image. So to handle this many-many relationship between the two, we could take use of the following tables: Table: Image Columns: Image_ID, Image_Title Table: Tag Columns: Tag_ID, Tag_Title Table: Image_Tag Columns: Tag_ID, Image_ID Setting the Image_ID and Tag_ID as foreign keys would help in linking the tables. Maybe this will help. Regards Swati
Apologies. Life has caught up with me a little these past months. Things are still busy but here are a few thoughts...
(In reply to Felix Leif Keppmann from comment #62) > > currently I face the issue of not being able to completely delete > directories. Directories are deleted on disk, but not deleted from the > database, i.e., obsolete ghost directories remain in Digikam's album tree. > > Seems to be related to failing MySQL queries: > > "UPDATE Albums SET albumRoot=0, relativePath=? WHERE id=?;" > Error messages: "QMYSQL3: Unable to execute statement" "Cannot add or update > a child row: a foreign key constraint fails (`digikam_core`.`Albums`, > CONSTRAINT `Albums_AlbumRoots` FOREIGN KEY (`albumRoot`) REFERENCES > `AlbumRoots` (`id`) ON DELETE CASCADE ON UPDATE CASCADE)" 1452 2 > Bound values: (QVariant(QString, "1-/test"), QVariant(int, 2)) > I think the issue here is that the database is trying to use albumRoot=0 to mark the album as being disconnected/deleted. However with referential integrity set this now is not allowed. This seems to be happening in CoreDB::makeStaleAlbum(int albumID) The lifecycle of stale albums needs considering a little more.
(In reply to swatilodha27 from comment #63) > Hello Richard. > > I'm Swati, currently working on digiKam MySQL database support project. I > read this open bug for MySQL Schema improvements and thought of a plausible > solution to handle the tags (rename, move or delete) > > An image can have many tags and a tag can belong to multiple image. So to > handle this many-many relationship between the two, we could take use of the > following tables: > > Table: Image > Columns: Image_ID, Image_Title > > Table: Tag > Columns: Tag_ID, Tag_Title > > Table: Image_Tag > Columns: Tag_ID, Image_ID > > Setting the Image_ID and Tag_ID as foreign keys would help in linking the > tables. > Foreign keys have already been added here. CONSTRAINT ImageTags_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE, CONSTRAINT ImageTags_Tags FOREIGN KEY (tagid) REFERENCES Tags (id) ON DELETE CASCADE ON UPDATE CASCADE, Without seeing a specific error I cannot tell what the problem may be with rename, move or delete.
Apart from testing and fixing issues I am aware of the following tasks that need completing for MySQL support. 1) Test and refine the "Experiment with TagsTree tables" patch to ensure that it maintains the tags tree correctly. 2) Add a migration script to upgrade existing MySQL databases with the referential integrity patches. 3) Look at image, album and tag rename, move and delete. The existing SQL assumes too many magic "0" values and intermediate steps where referential integrity would be broken. 4) Write unit tests for the various database operations. This should help with the above steps.
Hello Richard. I guess then the only possible way is to reproduce every error reported by the end user and try to generate it. Queries in relation to the points you mentioned for completing MySQL support: 1)In the "Experiment with TagsTree tables" patch, only testing needs to be done? To ensure that it generates expected results? 2) To add a script for migration of databases with referential integrity patches, would it require to create a new table in the database? I'm not actually clear on this point. 3) To accurately make image/album/tag function, referential integrity is the best possible solution in MySQL. To check if it is broken, multiple tables in DB need to be checked and figure out in which one a record is missing, which is present in other tables. I guess this could be a solution? Regards Swati
(In reply to swatilodha27 from comment #68) > Hello Richard. > > I guess then the only possible way is to reproduce every error reported by > the end user and try to generate it. Some errors may be obvious by just looking at the code. But reproducing things is definitely a good way to start. It also means that you can test the fix! > > Queries in relation to the points you mentioned for completing MySQL support: > 1)In the "Experiment with TagsTree tables" patch, only testing needs to be > done? To ensure that it generates expected results? Yes. I think it replicates existing behaviour. > > 2) To add a script for migration of databases with referential integrity > patches, would it require to create a new table in the database? I'm not > actually clear on this point. No. It just requires adding "alter table" commands to convert the previous schema to the new format. It should be pretty straightforward. But in many ways it is easier to do that when the referential integrity setup has been tested for new databases. During migration there is a chance that there will be some rows that have broken referential integrity. That said MySQL support has always been experimental so it is probably reasonable to expect that users may need to perform some cleanups to get an existing database to migrate to a non-experimental version. > > 3) To accurately make image/album/tag function, referential integrity is the > best possible solution in MySQL. To check if it is broken, multiple tables > in DB need to be checked and figure out in which one a record is missing, > which is present in other tables. I guess this could be a solution? Yes that is correct. But note that historically Digikam has not used a strict referential integrity solution. In a number of cases it uses zero instead of what should be null along with other "magic" values. In order to eliminate these from the database the actual C++ code will likely need changing to stop it relying on the magic zero values. Much of this code is shared with SQLite so care has to be taken to ensure that the new code works with both databases. Finding these locations will require some analysis of the code to audit all uses of a particular field with correction where required. The other issue (I'm guessing here based on what I've seen so far) is that digikam seems to perform two stage database operations in some cases. In the deletion example it seems to update the database to mark that an object is about to the deleted. Then it tries the deletion and if successful goes ahead and deletes the item from the database. In the case I looked at it seemed that referential integrity was broken between the two database operations and that is why the operation failed in MySQL. If my assessment is correct then some thought needs giving to how the operation can be performed in a database that enforces referential integrity. Regards Richard > > Regards > Swati
(In reply to Richard Mortimer from comment #69) > (In reply to swatilodha27 from comment #68) > > Hello Richard. > > > > I guess then the only possible way is to reproduce every error reported by > > the end user and try to generate it. > Some errors may be obvious by just looking at the code. But reproducing > things is definitely a good way to start. It also means that you can test > the fix! > > > > > Queries in relation to the points you mentioned for completing MySQL support: > > 1)In the "Experiment with TagsTree tables" patch, only testing needs to be > > done? To ensure that it generates expected results? > Yes. I think it replicates existing behaviour. > > > > > 2) To add a script for migration of databases with referential integrity > > patches, would it require to create a new table in the database? I'm not > > actually clear on this point. > No. It just requires adding "alter table" commands to convert the previous > schema to the new format. It should be pretty straightforward. But in many > ways it is easier to do that when the referential integrity setup has been > tested for new databases. > > During migration there is a chance that there will be some rows that have > broken referential integrity. That said MySQL support has always been > experimental so it is probably reasonable to expect that users may need to > perform some cleanups to get an existing database to migrate to a > non-experimental version. > > > > 3) To accurately make image/album/tag function, referential integrity is the > > best possible solution in MySQL. To check if it is broken, multiple tables > > in DB need to be checked and figure out in which one a record is missing, > > which is present in other tables. I guess this could be a solution? > Yes that is correct. > > But note that historically Digikam has not used a strict referential > integrity solution. In a number of cases it uses zero instead of what should > be null along with other "magic" values. In order to eliminate these from > the database the actual C++ code will likely need changing to stop it > relying on the magic zero values. Much of this code is shared with SQLite so > care has to be taken to ensure that the new code works with both databases. > Finding these locations will require some analysis of the code to audit all > uses of a particular field with correction where required. > To solve this maybe code could be reviewed in order to cross check that "0" isn't mentioned in ELSE condition, or in place of NULL. But I'm pretty confused on how to find locations where code needs to be changed, so as to make it compatible with both the databases? > The other issue (I'm guessing here based on what I've seen so far) is that > digikam seems to perform two stage database operations in some cases. In the > deletion example it seems to update the database to mark that an object is > about to the deleted. Then it tries the deletion and if successful goes > ahead and deletes the item from the database. In the case I looked at it > seemed that referential integrity was broken between the two database > operations and that is why the operation failed in MySQL. If my assessment > is correct then some thought needs giving to how the operation can be > performed in a database that enforces referential integrity. > Referential integrity can be enforced by strictly not violating any of one of the following: 1)No record should be allowed to add in the foreign key table, unless there exists a corresponding record in the superior table. 2) CASCADING UPDATE to automatically update corresponding record if the primary key record is updated. 3) CASCADING DELETE to automatically delete corresponding record if the primary key record is deleted. Regards Swati
Created attachment 99903 [details] Additional schema modifications from MySQL v7 to v8 Changes that need to be made for MySQL v7 to v8 transition in addition to those already listed. Note that I have not tested the transition from within digikam (I do not have a current working build tree)
Created attachment 99904 [details] Sample digikam v7 schema Here is my v7 based (digikam 4.x) MySQL schema. This may be useful for testing the upgrade from v7 to v8. I have tested the v7 upgrade commands against this (and also against a clone of my fully populated v7 database).
Created attachment 99905 [details] helper SQL command to detect invalid data in v7 MySQL databases running these commands against a v7 database will list any data that fails the referential integrity checks. It detects images attached to missing albums and similar for missing tags too.
Created attachment 99906 [details] Helper to remove bad data prior to upgrade fromMySQL v7 to v8 Take care using this and always make a backup of your database before applying this commands. The commands just remove any data that fails integrity checks on the grounds that is was meaningless without being properly linked into the image/album/tags structures.
Created attachment 99907 [details] Raw commands to perform a full MySQL v7 to v8 update These can be used to test upgrades from v7 to v8 standalone from digikam. Note that the commands do not update the internal database version numbers in the Settings table.
Created attachment 99908 [details] Add referential integrity constraints to a 5.0.0 v8 database The 5.0.0 initial release database upgrade does not add referential integrity to the database fields. The commands in this attachment perform those upgrades and complete the proper move from v7 to v8. Note that MySQL support in digikam has always been classed as experimental. You are advised to use the migration facilities to cleanup any MySQL import from an older version of the database.
Richard, I tested your patch for adding referential integrity constraints in 5.0.0 v8 DB and it seems to work fine for me. Thanks.
(In reply to swatilodha27 from comment #77) > Richard, > > I tested your patch for adding referential integrity constraints in 5.0.0 v8 > DB and it seems to work fine for me. > > Thanks. Thanks for testing.
Hi Richard Could you please suggest more needs to be done here, after your referential integrity patch has been implemented? Thanks
The only thing that I am aware of that potentially needs addressing in MySQL is the TagsTree support. I did note that there were some other issues addressing that so it may already have been resolved.
Hi Richard, Following issues have been fixed recently to ensure Tags works fine with MySQL support: 1) Removing _DigiKam_root_tag_ from Tags tree (https://github.com/KDE/digikam/commit/eb6318a45efe1e08f35da450633f3bcb1027fd6a) 2)Updating set fields (lft and rgt values) while moving tags in hierarchy (https://github.com/KDE/digikam/commit/b8df6225e3cafdd720120f4bad4150fd78525e02) And, if I'm not wrong, the lft/rgt IDs are used by search tools, and in other places where hierarchy is concerned ? What improvements were aimed with your Tags Tree patches? Is the current implementation sufficient to fix all the issues that you tried with your earlier patches ? Thank you!
Richard, Can we close this file since last Maik fixes for Mysql support ? Gilles Caulier
(In reply to caulier.gilles from comment #82) > Richard, > > Can we close this file since last Maik fixes for Mysql support ? > > Gilles Caulier Gilles, Yes I think we can. Richard