[PATCH 0/5] MySQL schema improvements

[Richard Mortimer]

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

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Richard Mortimer]

This manifests itself during database conversion where the transfer of
a record fails but the conversion carries on to other entries. The
typical result is a cascaded set of failures and corrupt data.
---
 libs/database/engine/dbenginebackend.cpp | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libs/database/engine/dbenginebackend.cpp b/libs/database/engine/dbenginebackend.cpp
index c5ea8fc..969786b 100644
--- a/libs/database/engine/dbenginebackend.cpp
+++ b/libs/database/engine/dbenginebackend.cpp
@@ -848,6 +848,8 @@ BdEngineBackend::QueryState BdEngineBackend::handleQueryResult(DbEngineSqlQuery&
         if (query.lastError().type() == QSqlError::ConnectionError)
         {
             return BdEngineBackend::ConnectionError;
+        } else {
+            return BdEngineBackend::SQLError;
         }
     }
 
--
2.5.0

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Richard Mortimer]

---
 libs/database/coredb/coredbcopymanager.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libs/database/coredb/coredbcopymanager.cpp b/libs/database/coredb/coredbcopymanager.cpp
index 7568126..8cbba79 100644
--- a/libs/database/coredb/coredbcopymanager.cpp
+++ b/libs/database/coredb/coredbcopymanager.cpp
@@ -110,7 +110,7 @@ void CoreDbCopyManager::copyDatabases(const DbEngineParameters& fromDBParameters
     QMap<QString, QVariant> bindingMap;
 
     // Delete all tables
-    for (int i=0; m_isStopProcessing || i < tablesSize; ++i)
+    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)
         {
--
2.5.0

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Richard Mortimer]

This allows use of referential integrity checks in the database schema.
---
 libs/database/coredb/coredbcopymanager.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libs/database/coredb/coredbcopymanager.cpp b/libs/database/coredb/coredbcopymanager.cpp
index 8cbba79..38c32a8 100644
--- a/libs/database/coredb/coredbcopymanager.cpp
+++ b/libs/database/coredb/coredbcopymanager.cpp
@@ -88,12 +88,12 @@ void CoreDbCopyManager::copyDatabases(const DbEngineParameters& fromDBParameters
         << QLatin1String("ImageHaarMatrix")
         << QLatin1String("ImageInformation")
         << QLatin1String("ImageMetadata")
-        << QLatin1String("ImageTagProperties")
-        << QLatin1String("TagProperties")
         << QLatin1String("ImagePositions")
         << QLatin1String("ImageComments")
         << QLatin1String("ImageCopyright")
         << QLatin1String("Tags")
+        << QLatin1String("TagProperties")
+        << QLatin1String("ImageTagProperties")
         << QLatin1String("ImageTags")
         << QLatin1String("ImageProperties")
         << QLatin1String("ImageHistory")
--
2.5.0

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Richard Mortimer]

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.
---
 data/database/dbconfig.xml.cmake.in | 82 ++++++++++++++++++-------------------
 1 file changed, 40 insertions(+), 42 deletions(-)

diff --git a/data/database/dbconfig.xml.cmake.in b/data/database/dbconfig.xml.cmake.in
index 42a6947..f2a951e 100644
--- a/data/database/dbconfig.xml.cmake.in
+++ b/data/database/dbconfig.xml.cmake.in
@@ -859,14 +859,12 @@
             <!-- Mysql check privileges actions -->
 
             <dbaction name="CheckPriv_CREATE_TRIGGER"><statement mode="plain">
-                CREATE TRIGGER privcheck_trigger AFTER DELETE ON PrivCheck
-                  FOR EACH ROW BEGIN
-                  END;
+                SELECT 1;
             </statement>
             </dbaction>
 
             <dbaction name="CheckPriv_DROP_TRIGGER"><statement mode="plain">
-                DROP TRIGGER privcheck_trigger;
+                SELECT 1;
             </statement>
             </dbaction>
 
@@ -913,6 +911,7 @@
                             caption LONGTEXT CHARACTER SET utf8,
                             collection LONGTEXT CHARACTER SET utf8,
                             icon INTEGER,
+                            CONSTRAINT Albums_AlbumRoots FOREIGN KEY (albumRoot) REFERENCES AlbumRoots (id) ON DELETE CASCADE ON UPDATE CASCADE,
                             UNIQUE(albumRoot, relativePath(255)));</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS Images
                             (id INTEGER PRIMARY KEY NOT NULL AUTO_INCREMENT,
@@ -923,12 +922,16 @@
                             modificationDate DATETIME,
                             fileSize INTEGER,
                             uniqueHash VARCHAR(128),
+                            CONSTRAINT Images_Albums FOREIGN KEY (album) REFERENCES Albums (id) ON DELETE CASCADE ON UPDATE CASCADE,
                             UNIQUE (album, name(255)));</statement>
+            <statement mode="plain">ALTER TABLE Albums
+                            ADD CONSTRAINT Albums_Images FOREIGN KEY (icon) REFERENCES Images (id) ON DELETE SET NULL ON UPDATE CASCADE;</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageHaarMatrix
                             (imageid INTEGER PRIMARY KEY,
                             modificationDate DATETIME,
                             uniqueHash LONGTEXT CHARACTER SET utf8,
-                            matrix LONGBLOB);</statement>
+                            matrix LONGBLOB,
+                            CONSTRAINT ImageHaarMatrix_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageInformation
                             (imageid INTEGER PRIMARY KEY,
                             rating INTEGER,
@@ -939,7 +942,8 @@
                             height INTEGER,
                             format LONGTEXT CHARACTER SET utf8,
                             colorDepth INTEGER,
-                            colorModel INTEGER);</statement>
+                            colorModel INTEGER,
+                            CONSTRAINT ImageInformation_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageMetadata
                             (imageid INTEGER PRIMARY KEY,
                             make LONGTEXT CHARACTER SET utf8,
@@ -957,7 +961,8 @@
                             whiteBalanceColorTemperature INTEGER,
                             meteringMode INTEGER,
                             subjectDistance REAL,
-                            subjectDistanceCategory INTEGER);</statement>
+                            subjectDistanceCategory INTEGER,
+                            CONSTRAINT ImageMetadata_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS VideoMetadata
                             (imageid INTEGER PRIMARY KEY,
                             aspectRatio TEXT,
@@ -967,7 +972,8 @@
                             duration TEXT,
                             frameRate TEXT,
                             exposureProgram INTEGER,
-                            videoCodec TEXT);</statement>
+                            videoCodec TEXT,
+                            CONSTRAINT VideoMetadata_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImagePositions
                             (imageid INTEGER PRIMARY KEY,
                             latitude LONGTEXT CHARACTER SET utf8,
@@ -979,7 +985,8 @@
                             tilt REAL,
                             roll REAL,
                             accuracy REAL,
-                            description LONGTEXT CHARACTER SET utf8);</statement>
+                            description LONGTEXT CHARACTER SET utf8,
+                            CONSTRAINT ImagePositions_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageComments
                             (id INTEGER PRIMARY KEY NOT NULL AUTO_INCREMENT,
                             imageid INTEGER,
@@ -988,6 +995,7 @@
                             author LONGTEXT CHARACTER SET utf8,
                             date DATETIME,
                             comment LONGTEXT CHARACTER SET utf8,
+                            CONSTRAINT ImageComments_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE,
                             UNIQUE(imageid, type, language, author(202)));</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageCopyright
                             (id INTEGER PRIMARY KEY NOT NULL AUTO_INCREMENT,
@@ -995,6 +1003,7 @@
                             property LONGTEXT CHARACTER SET utf8,
                             value LONGTEXT CHARACTER SET utf8,
                             extraValue LONGTEXT CHARACTER SET utf8,
+                            CONSTRAINT ImageCopyright_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE,
                             UNIQUE(imageid, property(110), value(111), extraValue(111)));</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS Tags
                             (id INTEGER PRIMARY KEY NOT NULL AUTO_INCREMENT,
@@ -1003,16 +1012,20 @@
                             icon INTEGER,
                             iconkde LONGTEXT CHARACTER SET utf8,
                             lft INT NOT NULL,
-                            rgt INT NOT NULL
+                            rgt INT NOT NULL,
+                            CONSTRAINT Tags_Images FOREIGN KEY (icon) REFERENCES Images (id) ON DELETE SET NULL ON UPDATE CASCADE
                             );</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageTags
                             (imageid INTEGER NOT NULL,
                             tagid INTEGER NOT NULL,
+                            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,
                             UNIQUE (imageid, tagid));</statement>
             <statement mode="plain"> CREATE TABLE IF NOT EXISTS ImageProperties
                             (imageid  INTEGER NOT NULL,
                             property LONGTEXT CHARACTER SET utf8    NOT NULL,
                             value    LONGTEXT CHARACTER SET utf8    NOT NULL,
+                            CONSTRAINT ImageProperties_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE,
                             UNIQUE (imageid, property(255)));</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS Searches
                             (id INTEGER PRIMARY KEY NOT NULL AUTO_INCREMENT,
@@ -1033,21 +1046,27 @@
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageHistory
                             (imageid INTEGER PRIMARY KEY,
                              uuid VARCHAR(128),
-                             history LONGTEXT CHARACTER SET utf8);</statement>
+                             history LONGTEXT CHARACTER SET utf8,
+                             CONSTRAINT ImageHistory_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageRelations
                             (subject INTEGER,
                              object INTEGER,
                              type INTEGER,
+                             CONSTRAINT ImageRelations_ImagesS FOREIGN KEY (subject) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE,
+                             CONSTRAINT ImageRelations_ImagesO FOREIGN KEY (object) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE,
                              UNIQUE(subject, object, type));</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS TagProperties
                             (tagid INTEGER,
                              property TEXT CHARACTER SET utf8,
-                             value LONGTEXT CHARACTER SET utf8);</statement>
+                             value LONGTEXT CHARACTER SET utf8,
+                             CONSTRAINT TagProperties_Tags FOREIGN KEY (tagid) REFERENCES Tags (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">CREATE TABLE IF NOT EXISTS ImageTagProperties
                             (imageid INTEGER,
                              tagid INTEGER,
                              property TEXT CHARACTER SET utf8,
-                             value LONGTEXT CHARACTER SET utf8);</statement>
+                             value LONGTEXT CHARACTER SET utf8,
+                             CONSTRAINT ImageTagProperties_Images FOREIGN KEY (imageid) REFERENCES Images (id) ON DELETE CASCADE ON UPDATE CASCADE,
+                             CONSTRAINT ImageTagProperties_Tags FOREIGN KEY (tagid) REFERENCES Tags (id) ON DELETE CASCADE ON UPDATE CASCADE);</statement>
             <statement mode="plain">
                             CREATE OR REPLACE VIEW TagsTree
                             AS
@@ -1109,34 +1128,9 @@
             <!-- Mysql Core Triggers -->
 
             <dbaction name="CreateTriggers" mode="transaction">
-            <statement mode="plain">DROP TRIGGER IF EXISTS delete_image;</statement>
-            <statement mode="plain">CREATE TRIGGER delete_image AFTER DELETE ON Images
-                    FOR EACH ROW BEGIN
-                        DELETE FROM ImageTags          WHERE imageid=OLD.id;
-                        DELETE From ImageHaarMatrix    WHERE imageid=OLD.id;
-                        DELETE From ImageInformation   WHERE imageid=OLD.id;
-                        DELETE From ImageMetadata      WHERE imageid=OLD.id;
-                        DELETE From VideoMetadata      WHERE imageid=OLD.id;
-                        DELETE From ImagePositions     WHERE imageid=OLD.id;
-                        DELETE From ImageComments      WHERE imageid=OLD.id;
-                        DELETE From ImageCopyright     WHERE imageid=OLD.id;
-                        DELETE From ImageProperties    WHERE imageid=OLD.id;
-                        DELETE From ImageHistory       WHERE imageid=OLD.id;
-                        DELETE FROM ImageRelations     WHERE subject=OLD.id OR object=OLD.id;
-                        DELETE FROM ImageTagProperties WHERE imageid=OLD.id;
-                        UPDATE Albums SET icon=null    WHERE icon=OLD.id;
-                        UPDATE Tags SET icon=null      WHERE icon=OLD.id;
-                    END;
-            </statement>
-            <statement mode="plain">DROP TRIGGER IF EXISTS delete_tag;</statement>
-            <statement mode="plain">CREATE TRIGGER delete_tag AFTER DELETE ON Tags
-            FOR EACH ROW BEGIN
-                DELETE FROM ImageTags          WHERE tagid=OLD.id;
-                DELETE FROM TagProperties      WHERE tagid=OLD.id;
-                DELETE FROM ImageTagProperties WHERE tagid=OLD.id;
-            END;
-            </statement>
+<!-- Note move_tagstree does not exist for mysql
             <statement mode="plain">DROP TRIGGER IF EXISTS move_tagstree;</statement>
+-->
 
             <statement mode="plain">SELECT
                     @minLeft := IF(ISNULL(MIN(lft)), 1, MIN(lft)-1),
@@ -1148,7 +1142,7 @@
             <statement mode="plain">REPLACE INTO Tags
                 (id, pid, name, icon, iconkde, lft, rgt)
                 VALUES
-                (0, -1, '_Digikam_root_tag_', 0, NULL, @minLeft, @maxRight )
+                (0, -1, '_Digikam_root_tag_', NULL, NULL, @minLeft, @maxRight )
             </statement>
             <statement mode="plain">SET SQL_MODE=@OLD_SQL_MODE;</statement>
             </dbaction>
@@ -1441,7 +1435,8 @@ ORDER BY inf.rating DESC, img.name ASC
                             (id INTEGER,
                             type INTEGER,
                             attribute TEXT,
-                            value TEXT);
+                            value TEXT,
+                            CONSTRAINT IdentityAttributes_Identities FOREIGN KEY (id) REFERENCES Identities (id) ON DELETE CASCADE ON UPDATE CASCADE);
                 </statement>
                 <statement mode="plain">CREATE TABLE IF NOT EXISTS Settings
                             (keyword LONGTEXT CHARACTER SET utf8 NOT NULL,
@@ -1523,12 +1518,14 @@ ORDER BY inf.rating DESC, img.name ASC
             <!-- Mysql Face Triggers -->
 
             <dbaction name="CreateFaceTriggers" mode="transaction">
+<!--
                 <statement mode="plain">DROP TRIGGER IF EXISTS delete_identities;</statement>
                 <statement mode="plain">CREATE TRIGGER delete_identities AFTER DELETE ON Identities
                 FOR EACH ROW BEGIN
                     DELETE FROM IdentityAttributes WHERE id=OLD.id;
                 END;
                 </statement>
+-->
             </dbaction>
 
             <!-- Mysql Migration Statements -->
@@ -1544,6 +1541,7 @@ ORDER BY inf.rating DESC, img.name ASC
             <dbaction name="Migrate_Read_Albums"><statement mode="query">
                 SELECT id, albumRoot, relativePath, date, caption, collection, icon FROM Albums;
             </statement></dbaction>
+            <!-- Note Albums with an icon set will probably fail to migrate due to referential integrity checks. The icons should probably be set after images are migrated. -->
             <dbaction name="Migrate_Write_Albums" mode="transaction"><statement mode="query">
                 INSERT INTO Albums (id, albumRoot, relativePath, date, caption, collection, icon) VALUES (:id, :albumRoot, :relativePath, :date, :caption, :collection, :icon);
             </statement></dbaction>
--
2.5.0

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel

[Richard Mortimer]

This will not work with SQLITE and needs moving into a dbconfig.xml clause.
---
 libs/database/coredb/coredbcopymanager.cpp | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libs/database/coredb/coredbcopymanager.cpp b/libs/database/coredb/coredbcopymanager.cpp
index 38c32a8..b312750 100644
--- a/libs/database/coredb/coredbcopymanager.cpp
+++ b/libs/database/coredb/coredbcopymanager.cpp
@@ -109,6 +109,9 @@ void CoreDbCopyManager::copyDatabases(const DbEngineParameters& fromDBParameters
 
     QMap<QString, QVariant> bindingMap;
 
+    // Drop foreign key on Albums.icon to allow proper delete
+    toDBbackend.execDirectSql(QString::fromUtf8("ALTER TABLE Albums DROP FOREIGN KEY Albums_Images;"));
+
     // Delete all tables
     for (int i=(tablesSize - 1); m_isStopProcessing || i >= 0; --i)
     {
--
2.5.0

_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel