[digikam] [Bug 355831] New: MySQL Schema Improvements

classic Classic list List threaded Threaded
92 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] New: MySQL Schema Improvements

bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

            Bug ID: 355831
           Summary: MySQL Schema Improvements
           Product: digikam
           Version: 5.0.0
          Platform: Compiled Sources
                OS: Linux
            Status: UNCONFIRMED
          Severity: wishlist
          Priority: NOR
         Component: Database-Schema
          Assignee: [hidden email]
          Reporter: [hidden email]

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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #1 from Richard Mortimer <[hidden email]> ---
Created attachment 95686
  --> https://bugs.kde.org/attachment.cgi?id=95686&action=edit
[PATCH 0/5] MySQL schema improvements

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #2 from Richard Mortimer <[hidden email]> ---
Created attachment 95687
  --> https://bugs.kde.org/attachment.cgi?id=95687&action=edit
[PATCH 1/5] Ensure that an error is returned if the query fails.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #3 from Richard Mortimer <[hidden email]> ---
Created attachment 95688
  --> https://bugs.kde.org/attachment.cgi?id=95688&action=edit
[PATCH 2/5] Delete tables in reverse creation order.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #4 from Richard Mortimer <[hidden email]> ---
Created attachment 95689
  --> https://bugs.kde.org/attachment.cgi?id=95689&action=edit
[PATCH 3/5] Ensure that data is migrated in line with foreign key

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #5 from Richard Mortimer <[hidden email]> ---
Created attachment 95690
  --> https://bugs.kde.org/attachment.cgi?id=95690&action=edit
[PATCH 4/5] Remove trigger dependency for MySQL.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #6 from Richard Mortimer <[hidden email]> ---
Created attachment 95691
  --> https://bugs.kde.org/attachment.cgi?id=95691&action=edit
[PATCH 5/5] Temporary workaround to break MySQL references loop with the Albums
icon entry.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #7 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #8 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]
  Attachment #95687|0                           |1
        is obsolete|                            |

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #95689|0                           |1
        is obsolete|                            |

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #95686|0                           |1
        is obsolete|                            |

--- Comment #9 from [hidden email] ---
Comment on attachment 95686
  --> https://bugs.kde.org/attachment.cgi?id=95686
[PATCH 0/5] MySQL schema improvements

>From 8cfb15da5f4de65a734aa864c68871822e144de1 Mon Sep 17 00:00:00 2001
>From: Richard Mortimer <[hidden email]>
>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
>

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #10 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

[hidden email] changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #95688|0                           |1
        is obsolete|                            |

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #11 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #12 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #13 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #14 from Richard Mortimer <[hidden email]> ---
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.

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #15 from [hidden email] ---
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 are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
Reply | Threaded
Open this post in threaded view
|

[digikam] [Bug 355831] MySQL Schema Improvements

bugzilla_noreply
In reply to this post by bugzilla_noreply
https://bugs.kde.org/show_bug.cgi?id=355831

--- Comment #16 from [hidden email] ---
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

--
You are receiving this mail because:
You are the assignee for the bug.
_______________________________________________
Digikam-devel mailing list
[hidden email]
https://mail.kde.org/mailman/listinfo/digikam-devel
12345