Bug 34064 - Compare kohastructure.sql against current database using database audit script
Summary: Compare kohastructure.sql against current database using database audit script
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Martin Renvoize
URL:
Keywords: dependency
Depends on:
Blocks: 36039
  Show dependency treegraph
 
Reported: 2023-06-20 04:17 UTC by David Cook
Modified: 2024-02-08 12:07 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 34064: Add an audit script to compare database with kohastructure.sql (3.84 KB, patch)
2023-06-20 04:28 UTC, David Cook
Details | Diff | Splinter Review
Bug 34064: Add an audit script to compare database with kohastructure.sql (4.23 KB, patch)
2023-06-20 05:29 UTC, David Cook
Details | Diff | Splinter Review
Bug 34064: Add an audit script to compare database with kohastructure.sql (4.29 KB, patch)
2023-06-20 05:58 UTC, Emmi Takkinen
Details | Diff | Splinter Review
Bug 34064: Remove no_batch_alters option (1.10 KB, patch)
2023-06-20 06:21 UTC, David Cook
Details | Diff | Splinter Review
Bug 34064: Add an audit script to compare database with kohastructure.sql (4.34 KB, patch)
2023-06-22 17:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34064: Remove no_batch_alters option (1.15 KB, patch)
2023-06-22 17:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34064: Add an audit script to compare database with kohastructure.sql (4.41 KB, patch)
2023-06-27 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34064: Remove no_batch_alters option (1.21 KB, patch)
2023-06-27 13:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 34064: Add SQL::Translator dependency to cpanfile (700 bytes, patch)
2023-07-05 05:48 UTC, David Cook
Details | Diff | Splinter Review
Bug 34064: Add SQL::Translator dependency to cpanfile (703 bytes, patch)
2023-07-05 05:55 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-06-20 04:17:17 UTC
As Marcel notes in bug 30486, it's common for an upgraded database to be inconsistent with kohastructure.sql.

It would be useful to have an audit script which could compare the current database with kohastructure.sql to see what differences there are.

--

Fortunately, I've come up with a script, which hopefully should get us most of the way there.
Comment 1 David Cook 2023-06-20 04:28:28 UTC Comment hidden (obsolete)
Comment 2 David Cook 2023-06-20 05:29:52 UTC Comment hidden (obsolete)
Comment 3 Emmi Takkinen 2023-06-20 05:58:06 UTC
Created attachment 152481 [details] [review]
Bug 34064: Add an audit script to compare database with kohastructure.sql

This script can take a connected database handle and compare that database
schema against kohastructure.sql to see what changes the database would
need in order to match kohastructure.sql

NOTE: It uses SQL::Translation::Diff, which is installed with DBIx::Class.

WARNING: The diff doesn't seem to compare comments, so that difference
won't appear in the output. If we wanted, we could easily enhance the
audit_database.pl script to also compare comments.

WARNING: The output is a proposed series of SQL commands. While they
are useful to review, they won't always duplicate the changes done
by updatedatabase.pl, so it's important to carefully analyze the output.
The key purpose of this audit script is to just highlight the differences
between the two.

Test plan:
0. Apply patch
1. vi ./installer/data/mysql/kohastructure.sql
2. Comment out some columns, change NULL status, or whatever you like
3. perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql
4. Note that the output includes SQL commands to change the database
to match the new kohastructure.sql

5a. Try using koha-foreach and note that the database name appears above
the database comparison
5b. koha-foreach "perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql"

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Comment 4 David Cook 2023-06-20 06:19:12 UTC
Thanks, Emmi, although I'm going to make a little tweak.

I've been running this on a backup server, and finding the output too unwieldy without the tweak.

It's amazing looking at real world databases though. My first database has about 1400 lines of suggested changes. 

Lots of the changes are for little things like synchronizing column collations, but it just goes to show how different updatedatabase.pl is from kohastructure.sql...
Comment 5 David Cook 2023-06-20 06:21:52 UTC
Created attachment 152482 [details] [review]
Bug 34064: Remove no_batch_alters option

This change removes the no_batch_alters option.

When working with real world databases, you really want to use batch
alters to group together changes. Otherwise, the output becomes
too overwhelming/unwieldy.

On real world databases, I'm noticing big discrepancies, although
I think most of the time this is a difference in the collation
at the column level.
Comment 6 David Cook 2023-06-20 06:23:09 UTC
On a built server it would be:

koha-foreach "perl /usr/share/koha/bin/maintenance/audit_database.pl --filename /usr/share/koha/intranet/cgi-bin/installer/data/mysql/kohastructure.sql" > audit_database.log
Comment 7 Jonathan Druart 2023-06-20 06:27:32 UTC
Why not using koha-dump?
With misc/maintenance/sync_db_comments.pl you could then compare with kohastructure.sql I think.
Comment 8 Julian Maurice 2023-06-20 07:11:28 UTC
I think it doesn't work for me. It tells me to change all varchar/text column. I tried executing one of these queries and ran the script again: this one query still shows up.
Comment 9 Emmi Takkinen 2023-06-20 09:18:14 UTC
(In reply to David Cook from comment #4)
> Thanks, Emmi, although I'm going to make a little tweak.
> 
> I've been running this on a backup server, and finding the output too
> unwieldy without the tweak.
> 
> It's amazing looking at real world databases though. My first database has
> about 1400 lines of suggested changes. 
> 
> Lots of the changes are for little things like synchronizing column
> collations, but it just goes to show how different updatedatabase.pl is from
> kohastructure.sql...

Oh boy. And here I was thinking that my database has a lot of changes to consider (maybe around 200 lines). Although, mine isn't as cleaned up as our production databases. Would be interesting to see how those still differ from kohastructure.sql.
Comment 10 Marcel de Rooy 2023-06-20 11:05:40 UTC
(In reply to Jonathan Druart from comment #7)
> Why not using koha-dump?
> With misc/maintenance/sync_db_comments.pl you could then compare with
> kohastructure.sql I think.

Yes, we are already having code to do this job or similar code that we could merge instead of adding more?
Comment 11 Marcel de Rooy 2023-06-20 11:19:42 UTC
Another example (code that I added :) is Koha/Database/Commenter.
The associated script helps you to get rid of all changed comment cruft and only focus on the real db changes.
Comment 12 David Cook 2023-06-20 23:40:04 UTC
(In reply to Jonathan Druart from comment #7)
> Why not using koha-dump?
> With misc/maintenance/sync_db_comments.pl you could then compare with
> kohastructure.sql I think.

You mean comparing a schema dump and kohastructure.sql? That could work, but it puts a lot of work on the human brain I think.
Comment 13 David Cook 2023-06-20 23:40:57 UTC
(In reply to Julian Maurice from comment #8)
> I think it doesn't work for me. It tells me to change all varchar/text
> column. I tried executing one of these queries and ran the script again:
> this one query still shows up.

It's probably related to collation or character set. I was a bit confused at first by some of the suggestions, and that's what it ended up being.

Which query still shows up?
Comment 14 David Cook 2023-06-20 23:41:49 UTC
(In reply to Emmi Takkinen from comment #9)
> Oh boy. And here I was thinking that my database has a lot of changes to
> consider (maybe around 200 lines). Although, mine isn't as cleaned up as our
> production databases. Would be interesting to see how those still differ
> from kohastructure.sql.

Right? I'm finding the discrepancies very interesting.
Comment 15 David Cook 2023-06-20 23:53:20 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Jonathan Druart from comment #7)
> > Why not using koha-dump?
> > With misc/maintenance/sync_db_comments.pl you could then compare with
> > kohastructure.sql I think.
> 
> Yes, we are already having code to do this job or similar code that we could
> merge instead of adding more?

(In reply to Marcel de Rooy from comment #11)
> Another example (code that I added :) is Koha/Database/Commenter.
> The associated script helps you to get rid of all changed comment cruft and
> only focus on the real db changes.

I did take a look at Koha::Database::Commenter, but it is specific to comments and it's not fully parsing the SQL. 

SQL::Translator is a great externally maintained library that makes it easy to programmatically work with database schemas, and it's already a dependency of the DBIx::Class deb package.

Koha::Database::Commenter could be replaced with a SQL::Translator based mechanism.
Comment 16 David Cook 2023-06-21 00:07:08 UTC
I'm a bit disappointed that people aren't as excited about this script as I am, but that's OK. 

I wrote it to help me fix my local databases, and it's already showing me lots of differences. I can keep this as a local tool, and use it raise Bugzilla issues. I just thought the community would find it useful, since this is an area that we've really struggled with together for many years.
Comment 17 Owen Leonard 2023-06-22 17:15:24 UTC
Created attachment 152575 [details] [review]
Bug 34064: Add an audit script to compare database with kohastructure.sql

This script can take a connected database handle and compare that database
schema against kohastructure.sql to see what changes the database would
need in order to match kohastructure.sql

NOTE: It uses SQL::Translation::Diff, which is installed with DBIx::Class.

WARNING: The diff doesn't seem to compare comments, so that difference
won't appear in the output. If we wanted, we could easily enhance the
audit_database.pl script to also compare comments.

WARNING: The output is a proposed series of SQL commands. While they
are useful to review, they won't always duplicate the changes done
by updatedatabase.pl, so it's important to carefully analyze the output.
The key purpose of this audit script is to just highlight the differences
between the two.

Test plan:
0. Apply patch
1. vi ./installer/data/mysql/kohastructure.sql
2. Comment out some columns, change NULL status, or whatever you like
3. perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql
4. Note that the output includes SQL commands to change the database
to match the new kohastructure.sql

5a. Try using koha-foreach and note that the database name appears above
the database comparison
5b. koha-foreach "perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql"

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 18 Owen Leonard 2023-06-22 17:15:28 UTC
Created attachment 152576 [details] [review]
Bug 34064: Remove no_batch_alters option

This change removes the no_batch_alters option.

When working with real world databases, you really want to use batch
alters to group together changes. Otherwise, the output becomes
too overwhelming/unwieldy.

On real world databases, I'm noticing big discrepancies, although
I think most of the time this is a difference in the collation
at the column level.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 19 Owen Leonard 2023-06-22 17:17:59 UTC
A note about my testing: I did not perform the test using koha-foreach, only on my single test database.

The database I use for testing is an older one from our production system and it has been updated again and again. I needed a tool today to figure out whether my problems importing patrons were Koha's fault or my database's, and this tool identified a number of problems which I was able to resolve. Now I can test patron imports again.

Note: I changed one spelling error identified by the QA tool during signoff: unecessarily -> unnecessarily
Comment 20 David Cook 2023-06-22 23:38:32 UTC
(In reply to Owen Leonard from comment #19)
> The database I use for testing is an older one from our production system
> and it has been updated again and again. I needed a tool today to figure out
> whether my problems importing patrons were Koha's fault or my database's,
> and this tool identified a number of problems which I was able to resolve.
> Now I can test patron imports again.

That's awesome, Owen! You made my day :).
Comment 21 Martin Renvoize 2023-06-27 13:31:23 UTC
Created attachment 152739 [details] [review]
Bug 34064: Add an audit script to compare database with kohastructure.sql

This script can take a connected database handle and compare that database
schema against kohastructure.sql to see what changes the database would
need in order to match kohastructure.sql

NOTE: It uses SQL::Translation::Diff, which is installed with DBIx::Class.

WARNING: The diff doesn't seem to compare comments, so that difference
won't appear in the output. If we wanted, we could easily enhance the
audit_database.pl script to also compare comments.

WARNING: The output is a proposed series of SQL commands. While they
are useful to review, they won't always duplicate the changes done
by updatedatabase.pl, so it's important to carefully analyze the output.
The key purpose of this audit script is to just highlight the differences
between the two.

Test plan:
0. Apply patch
1. vi ./installer/data/mysql/kohastructure.sql
2. Comment out some columns, change NULL status, or whatever you like
3. perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql
4. Note that the output includes SQL commands to change the database
to match the new kohastructure.sql

5a. Try using koha-foreach and note that the database name appears above
the database comparison
5b. koha-foreach "perl misc/maintenance/audit_database.pl \
    --filename /kohadevbox/koha/installer/data/mysql/kohastructure.sql"

Signed-off-by: Emmi Takkinen <emmi.takkinen@koha-suomi.fi>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2023-06-27 13:31:26 UTC
Created attachment 152740 [details] [review]
Bug 34064: Remove no_batch_alters option

This change removes the no_batch_alters option.

When working with real world databases, you really want to use batch
alters to group together changes. Otherwise, the output becomes
too overwhelming/unwieldy.

On real world databases, I'm noticing big discrepancies, although
I think most of the time this is a difference in the collation
at the column level.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Martin Renvoize 2023-06-27 13:32:20 UTC
I think this is actually really rather useful to admins.. it's a simple way to do the compare of dumps that Jonathan suggests, but in a nice report.

Script works, doesn't cause any harm.. I'm going to PQA :)
Comment 24 Tomás Cohen Arazi 2023-07-03 14:59:29 UTC
Mason, are we good to go with the dependencies?

SQL::Translator
SQL::Translator::Diff

Marking blocked until then.
Comment 25 David Cook 2023-07-03 22:59:38 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> Mason, are we good to go with the dependencies?
> 
> SQL::Translator
> SQL::Translator::Diff
> 
> Marking blocked until then.

They're dependencies of DBIx::Class, but probably a good idea to explicitly add them as well.
Comment 26 David Cook 2023-07-03 23:00:08 UTC
(In reply to David Cook from comment #25)
> They're dependencies of DBIx::Class, but probably a good idea to explicitly
> add them as well.

Or rather they're dependencies of the Debian package.
Comment 27 Tomás Cohen Arazi 2023-07-03 23:17:25 UTC
(In reply to David Cook from comment #26)
> (In reply to David Cook from comment #25)
> > They're dependencies of DBIx::Class, but probably a good idea to explicitly
> > add them as well.
> 
> Or rather they're dependencies of the Debian package.

I checked. And `pmvers` would tell me the Diff one was not installed.
Comment 28 David Cook 2023-07-04 00:07:48 UTC
(In reply to Tomás Cohen Arazi from comment #27)
> (In reply to David Cook from comment #26)
> > (In reply to David Cook from comment #25)
> > > They're dependencies of DBIx::Class, but probably a good idea to explicitly
> > > add them as well.
> > 
> > Or rather they're dependencies of the Debian package.
> 
> I checked. And `pmvers` would tell me the Diff one was not installed.

Run "dpkg -L libsql-translator-perl". You'll see them both in the output:

/usr/share/perl5/SQL/Translator/Diff.pm
/usr/share/perl5/SQL/Translator.pm

And I made a mistake about the dependencies. It's not a dependency of libdbix-class-perl. It's a dependency of "libtest-dbix-class-perl". 

At the moment "libtest-dbix-class-perl" is a dependency of koha-common, but it's one that I'd like to remove one day, so it's worth adding "libsql-translator-perl" as a Koha dependency.

But production Koha systems definitely already have both SQL::Translator and SQL::Translator::Diff installed because they have "libtest-dbix-class-perl" installed.
Comment 29 David Cook 2023-07-04 00:09:09 UTC
(In reply to Tomás Cohen Arazi from comment #27)
> I checked. And `pmvers` would tell me the Diff one was not installed.

`pmvers` doesn't work for SQL::Translator::Diff, since that package doesn't have a $VERSION variable.
Comment 30 Marcel de Rooy 2023-07-04 11:07:11 UTC
(In reply to David Cook from comment #28)
> At the moment "libtest-dbix-class-perl" is a dependency of koha-common, but
> it's one that I'd like to remove one day, so it's worth adding
> "libsql-translator-perl" as a Koha dependency.

Just do it. Love to hear that :)
Comment 31 David Cook 2023-07-05 05:48:20 UTC
Created attachment 153022 [details] [review]
Bug 34064: Add SQL::Translator dependency to cpanfile
Comment 32 David Cook 2023-07-05 05:53:13 UTC
Worth noting that SQL::Translator 1.62 is available from Debian 11 and Ubuntu 20.04.

While we've dropped Ubuntu 18.04 from https://wiki.koha-community.org/wiki/System_requirements_and_recommendations we haven't dropped Debian 10. While Debian 10 has passed it's normal EOL, it appears to have EOL LTS to 2024-06-30.

This patch was tested against SQL::Translator 1.62, so that's what I've put into the cpanfile patch.  

--

We could either lower the cpanfile requirement to 0.11024 (the version in Debian 10 and Ubuntu 18.04) and test, or we could package SQL::Translator 1.62 for Debian 10 Buster. 

While the version numbers are very different, the changes don't look massive: https://metacpan.org/dist/SQL-Translator/changes
Comment 34 David Cook 2023-07-05 05:55:28 UTC
Created attachment 153023 [details] [review]
Bug 34064: Add SQL::Translator dependency to cpanfile
Comment 35 David Cook 2023-09-06 05:49:23 UTC
So I think we've figured out the dependency thing. Should this still be BLOCKED or should it go back to "Passed QA"?
Comment 36 Martin Renvoize 2023-09-08 13:29:13 UTC
I think it should be back to PQA :).. setting as such.
Comment 37 Tomás Cohen Arazi 2023-10-10 13:56:16 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 38 Fridolin Somers 2023-10-12 21:02:11 UTC
Enhancement not pushed to 23.05.x

Looks great !