Bug 26947 - kohastructure.sql should be updated for each release
Summary: kohastructure.sql should be updated for each release
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 28471 28749
  Show dependency treegraph
 
Reported: 2020-11-06 11:41 UTC by Nick Clemens
Modified: 2023-12-28 20:43 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
The kohastructure file which contains the table create statements for Koha was originally a MySQL dump which has been managed by hand as developers added new tables, changed columns, etc. In order to standardize this table and allow easy comparison of an existing sites database to the correct Koha structure this file should be automatically generated as part of the release process
Version(s) released in:
21.05.00


Attachments
Bug 26947: Add scheam-only option to koha-dump (4.81 KB, patch)
2020-11-13 13:53 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26947: (follow-up) Set key checks to disabled for kohastructure (2.15 KB, patch)
2020-11-18 17:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26947: Move -- comment to the COMMENT clause (257.64 KB, patch)
2020-11-19 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Replace double quotes from comments (3.07 KB, patch)
2020-11-19 10:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Add scheam-only option to koha-dump (4.86 KB, patch)
2020-11-19 10:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26947: (follow-up) Set key checks to disabled for kohastructure (1.65 KB, patch)
2020-11-19 10:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26947: Move -- comment to the COMMENT clause (257.70 KB, patch)
2020-11-19 10:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26947: Replace double quotes from comments (3.14 KB, patch)
2020-11-19 10:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26947: Correct path for chown and chmod (996 bytes, patch)
2020-11-25 14:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Add scheam-only option to koha-dump (4.92 KB, patch)
2020-11-26 19:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26947: (follow-up) Set key checks to disabled for kohastructure (1.70 KB, patch)
2020-11-26 19:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26947: Move -- comment to the COMMENT clause (257.76 KB, patch)
2020-11-26 19:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26947: Replace double quotes from comments (3.19 KB, patch)
2020-11-26 19:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26947: Correct path for chown and chmod (1.03 KB, patch)
2020-11-26 19:04 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26947: Add scheam-only option to koha-dump (4.99 KB, patch)
2020-12-04 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26947: (follow-up) Set key checks to disabled for kohastructure (1.77 KB, patch)
2020-12-04 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26947: Move -- comment to the COMMENT clause (257.83 KB, patch)
2020-12-04 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26947: Replace double quotes from comments (3.26 KB, patch)
2020-12-04 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26947: Correct path for chown and chmod (1.10 KB, patch)
2020-12-04 14:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 26947: Update kohastructure.sql (566.28 KB, patch)
2020-12-04 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Add schema-only option to koha-dump (5.12 KB, patch)
2021-01-12 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: (follow-up) Set key checks to disabled for kohastructure (1.76 KB, patch)
2021-01-12 10:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Move -- comment to the COMMENT clause (257.81 KB, patch)
2021-01-12 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Replace double quotes from comments (3.25 KB, patch)
2021-01-12 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Correct path for chown and chmod (1.08 KB, patch)
2021-01-12 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Update kohastructure.sql (566.28 KB, patch)
2021-01-12 10:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26947: Remove AUTO_INCREMENT (18.47 KB, patch)
2021-01-12 11:31 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-11-06 11:41:20 UTC
The kohastructure.sql was originally a dump fo the database schema. Over time, however, it has been manually updated by developers as the tables were altered.

Currently the file is out of order, and the conventions for punctuation and  formatting are not standard. For instance, some numbers are quoted, some tables names are in backticks, some foreign keys are listed as 'CONSTRAINT' and some aren't

The advantage to regenerating the file is that then users can dump their db schema and easily diff it against the expected structure to see what might be missing.

Many DB updates simply don't add a new constraint if the existing data violates this, and as we use more Koha Objects, these missing constraints cause issues.

We can add a script to kohadevbox or the release tools to make it easy to generate the new file and can add as a step in the release process
Comment 1 Nick Clemens 2020-11-13 13:53:30 UTC
Created attachment 113603 [details] [review]
Bug 26947: Add scheam-only option to koha-dump

This patch adds a new --schema-only optoin to the koha-dump script.

This can be used to generate a new kohastructure.sql file during the release, and later to
compare the DB structure of a koha instance to the kohastructure.sql file shipped with a version
to identify any missing constraints or other db structure issues

To test:
1 - debian/scripts/koha-dump kohadev
2 - Confirm db and configs are dumped correctly
3 - Apply patch
4 - debian/scripts/koha-dump --help
5 - Confirm new option is listed and makes sense
6 - debian/scripts/koha-dump --schema-only kohadev
7 - Confirm only schema is dumped and is not zipped
8 - debian/scripts/koha-dump kohadev
9 - Confirm entire db is dumped as before
Comment 2 Jonathan Druart 2020-11-17 14:20:03 UTC
On koha-testing-docker:

root@kohadevbox:koha(master)$ perl debian/scripts/koha-dump --schema-only kohadev
Dumping Koha site kohadev:
* schema to /var/spool/koha/kohadev/kohadev-schema-2020-11-17.sql
mysqldump: Error: 'Access denied; you need (at least one of) the PROCESS privilege(s) for this operation' when trying to dump tablespaces

Should not we have a bash function in ktd instead? Or a dev script in misc/devel?
Comment 3 Jonathan Druart 2020-11-17 14:26:44 UTC
(In reply to Jonathan Druart from comment #2)
> On koha-testing-docker:
> 
> root@kohadevbox:koha(master)$ perl debian/scripts/koha-dump --schema-only
> kohadev
> Dumping Koha site kohadev:
> * schema to /var/spool/koha/kohadev/kohadev-schema-2020-11-17.sql
> mysqldump: Error: 'Access denied; you need (at least one of) the PROCESS
> privilege(s) for this operation' when trying to dump tablespaces

I was using MySQL8 and had to
> GRANT USAGE, PROCESS ON *.* TO `koha_kohadev`@`%`;
Comment 4 Jonathan Druart 2020-11-17 14:28:59 UTC
(In reply to Jonathan Druart from comment #2)
> Should not we have a bash function in ktd instead? Or a dev script in
> misc/devel?

function update_kohastructure {
    mysqldump --databases -d --host=db --user=koha_kohadev --password=password koha_kohadev > installer/data/mysql/kohastructure.sql
}
Comment 5 Jonathan Druart 2020-11-17 14:44:17 UTC
C4::Installer::load_sql returned the following errors while attempting to load /kohadevbox/koha/installer/data/mysql/kohastructure.sql:
DBD::mysql::st execute failed: Failed to open the referenced table 'branches' at /usr/share/perl5/DBIx/RunSQL.pm line 273.
Something went wrong loading file /kohadevbox/koha/installer/data/mysql/kohastructure.sql ([SQL ERROR]: CREATE TABLE `account_credit_types_branches` (

Did it work for you?
Comment 6 Jonathan Druart 2020-11-18 15:52:38 UTC
(In reply to Jonathan Druart from comment #5)
> C4::Installer::load_sql returned the following errors while attempting to
> load /kohadevbox/koha/installer/data/mysql/kohastructure.sql:
> DBD::mysql::st execute failed: Failed to open the referenced table
> 'branches' at /usr/share/perl5/DBIx/RunSQL.pm line 273.
> Something went wrong loading file
> /kohadevbox/koha/installer/data/mysql/kohastructure.sql ([SQL ERROR]: CREATE
> TABLE `account_credit_types_branches` (
> 
> Did it work for you?

Actually, the following works:

> DROP DATABASE koha_kohadev
> CREATE DATABASE koha_kohadev
koha-mysql kohadev < installer/data/mysql/kohastructure.sql

But reset_all does not, neither the installer (it says "Database tables created", but it's wrong...)
Comment 7 Jonathan Druart 2020-11-18 15:59:15 UTC
My guess is that the following instruction line, from kohastructure.sql, is not read by DBIx::RunSQL

  14 /*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 */;

we load the schema from C4::Installer
282     my $error = $self->load_sql("$datadir/kohastructure.sql")
Comment 8 Martin Renvoize 2020-11-18 16:09:08 UTC
Something along the line 'sed -r --in-place 's/,\s*-- (.*)$/ COMMENT "\1",/g;' installer/data/mysql/kohastructure.sql' should be used before the first run of this to capture our existing `, -- this is a comment` type comments and convert them to `COMMENT 'this is a comment',` so we keep the data and it also gets added to DBIC schemas as per the research undertaken by Victor: https://wiki.koha-community.org/wiki/User:Victor_Grousset_-_tuxayo/Coding_Guidelines-SQL11-changes
Comment 9 Jonathan Druart 2020-11-18 16:32:52 UTC
I didn't find a way to export the schema in an order different than alpha. I may have missed something somewhere... but I read the mysqldump man page (as well as mydumper) and didn't find anything. I guess it's not possible as you could have two tables having each one relation to the other.
Comment 10 Martin Renvoize 2020-11-18 17:16:03 UTC
Created attachment 113794 [details] [review]
Bug 26947: (follow-up) Set key checks to disabled for kohastructure
Comment 11 Martin Renvoize 2020-11-18 17:18:34 UTC
Not entirely sure I like this approach.. it's very MySQL centric.. even MariaDB complains about some of the statements :(.. we can probably safely drop those, but I wasn't 100% sure what they did or why they appear in the dumpfile in the first place.

The alternative is to fallback to using the system MySQL cli from the perl script.. but I don't really like that idea much either... so perhaps just falling back to disabling foreign key checks is the right thing to do here.
Comment 12 Jonathan Druart 2020-11-19 09:47:54 UTC
Should not we only need to turn off FOREIGN_KEY_CHECKS?
Comment 13 Martin Renvoize 2020-11-19 09:54:18 UTC
(In reply to Jonathan Druart from comment #12)
> Should not we only need to turn off FOREIGN_KEY_CHECKS?

I think you're probably right, I wasn't really sure why mysqldump turns so much else off.. I aim to try and understand that today and will quickly followup with details if I find any and likely re-submit that final patch with just the disablement of keys rather than all the other oddities.
Comment 14 Jonathan Druart 2020-11-19 10:11:07 UTC
Created attachment 113799 [details] [review]
Bug 26947: Move -- comment to the COMMENT clause

Sed command from bug 26947 comment 8
sed -r --in-place 's/,\s*-- (.*)$/ COMMENT "\1",/g;' installer/data/mysql/kohastructure.sql
Comment 15 Jonathan Druart 2020-11-19 10:11:11 UTC
Created attachment 113800 [details] [review]
Bug 26947: Replace double quotes from comments
Comment 16 Jonathan Druart 2020-11-19 10:11:54 UTC
Martin, I wanted to try your sed command, it works pretty well!
I only had to adjust manually 3 occurrences. Attaching the patches here but feel free to drop them if needed.
Comment 17 Martin Renvoize 2020-11-19 10:45:51 UTC
Created attachment 113802 [details] [review]
Bug 26947: Add scheam-only option to koha-dump

This patch adds a new --schema-only optoin to the koha-dump script.

This can be used to generate a new kohastructure.sql file during the release, and later to
compare the DB structure of a koha instance to the kohastructure.sql file shipped with a version
to identify any missing constraints or other db structure issues

To test:
1 - debian/scripts/koha-dump kohadev
2 - Confirm db and configs are dumped correctly
3 - Apply patch
4 - debian/scripts/koha-dump --help
5 - Confirm new option is listed and makes sense
6 - debian/scripts/koha-dump --schema-only kohadev
7 - Confirm only schema is dumped and is not zipped
8 - debian/scripts/koha-dump kohadev
9 - Confirm entire db is dumped as before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2020-11-19 10:45:55 UTC
Created attachment 113803 [details] [review]
Bug 26947: (follow-up) Set key checks to disabled for kohastructure

This patch adds sql calls to disable foreign key checks for the initial
kohastructure schema load and then re-enables them again for subsequent
actions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2020-11-19 10:45:59 UTC
Created attachment 113804 [details] [review]
Bug 26947: Move -- comment to the COMMENT clause

Sed command from bug 26947 comment 8
sed -r --in-place 's/,\s*-- (.*)$/ COMMENT "\1",/g;' installer/data/mysql/kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2020-11-19 10:46:02 UTC
Created attachment 113805 [details] [review]
Bug 26947: Replace double quotes from comments

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Jonathan Druart 2020-11-19 10:59:08 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Jonathan Druart from comment #2)
> > Should not we have a bash function in ktd instead? Or a dev script in
> > misc/devel?
> 
> function update_kohastructure {
>     mysqldump --databases -d --host=db --user=koha_kohadev
> --password=password koha_kohadev > installer/data/mysql/kohastructure.sql
> }

This one is better, but still not perfect:
mysqldump --no-create-db --no-data --host=db --user=koha_kohadev --password=password koha_kohadev > installer/data/mysql/kohastructure.sql
Comment 22 Victor Grousset/tuxayo 2020-11-19 22:29:07 UTC
With the patch, the dump doesn't have the following part, is that expected?

-- Current Database: `koha_kohadev`                           
--

CREATE DATABASE /*!32312 IF NOT EXISTS*/ `koha_kohadev` /*!40100 DEFAULT CHARACTER SET latin1 */;

USE `koha_kohadev`;

--
Comment 23 Jonathan Druart 2020-11-24 13:52:43 UTC
(In reply to Victor Grousset/tuxayo from comment #22)
> With the patch, the dump doesn't have the following part, is that expected?
> 
> -- Current Database: `koha_kohadev`                           
> --
> 
> CREATE DATABASE /*!32312 IF NOT EXISTS*/ `koha_kohadev` /*!40100 DEFAULT
> CHARACTER SET latin1 */;
> 
> USE `koha_kohadev`;
> 
> --

Yes.
Comment 24 Jonathan Druart 2020-11-24 14:09:32 UTC
Nick, Martin, what is your opinion about koha-dump vs the bash alias?

Or even a simple bash script in misc/devel that would generate it?

I think the following line is more simple and maintainable than tweaking koha-dump

mysqldump --no-create-db --no-data --host=db --user=koha_kohadev --password=password koha_kohadev
Comment 25 Nick Clemens 2020-11-24 14:42:40 UTC
(In reply to Jonathan Druart from comment #24)
> Nick, Martin, what is your opinion about koha-dump vs the bash alias?
> 
> Or even a simple bash script in misc/devel that would generate it?
> 
> I think the following line is more simple and maintainable than tweaking
> koha-dump
> 
> mysqldump --no-create-db --no-data --host=db --user=koha_kohadev
> --password=password koha_kohadev

The reason I like koha-dump is that it allows for easy dumping of the existing database for comparison - it provides a tool that is the same used during releasing

An alias/script can do the same, but isn't quite as easy I think. What is the argument against koha-dump?
Comment 26 Jonathan Druart 2020-11-24 15:03:54 UTC
(In reply to Nick Clemens from comment #25)
> An alias/script can do the same, but isn't quite as easy I think. What is
> the argument against koha-dump?

43 insertions(+), 26 deletions(-) in your patch, vs a 1 line change if we have an alias. I am ok with your changes if you think it will be easier as it.
Comment 27 Jonathan Druart 2020-11-24 15:05:28 UTC
And also make a 3rd place for RM tools (we already have the release-tools repo and the misc/devel dir).
Comment 28 Jonathan Druart 2020-11-25 14:44:43 UTC
Created attachment 113989 [details] [review]
Bug 26947: Correct path for chown and chmod
Comment 29 Victor Grousset/tuxayo 2020-11-26 19:04:22 UTC
Created attachment 114021 [details] [review]
Bug 26947: Add scheam-only option to koha-dump

This patch adds a new --schema-only optoin to the koha-dump script.

This can be used to generate a new kohastructure.sql file during the release, and later to
compare the DB structure of a koha instance to the kohastructure.sql file shipped with a version
to identify any missing constraints or other db structure issues

To test:
1 - debian/scripts/koha-dump kohadev
2 - Confirm db and configs are dumped correctly
3 - Apply patch
4 - debian/scripts/koha-dump --help
5 - Confirm new option is listed and makes sense
6 - debian/scripts/koha-dump --schema-only kohadev
7 - Confirm only schema is dumped and is not zipped
8 - debian/scripts/koha-dump kohadev
9 - Confirm entire db is dumped as before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 30 Victor Grousset/tuxayo 2020-11-26 19:04:25 UTC
Created attachment 114022 [details] [review]
Bug 26947: (follow-up) Set key checks to disabled for kohastructure

This patch adds sql calls to disable foreign key checks for the initial
kohastructure schema load and then re-enables them again for subsequent
actions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 31 Victor Grousset/tuxayo 2020-11-26 19:04:29 UTC
Created attachment 114023 [details] [review]
Bug 26947: Move -- comment to the COMMENT clause

Sed command from bug 26947 comment 8
sed -r --in-place 's/,\s*-- (.*)$/ COMMENT "\1",/g;' installer/data/mysql/kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 32 Victor Grousset/tuxayo 2020-11-26 19:04:33 UTC
Created attachment 114024 [details] [review]
Bug 26947: Replace double quotes from comments

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 33 Victor Grousset/tuxayo 2020-11-26 19:04:36 UTC
Created attachment 114025 [details] [review]
Bug 26947: Correct path for chown and chmod

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 34 Victor Grousset/tuxayo 2020-11-26 19:12:30 UTC
As an help for QA, here is another signoff.
QA script ok

Note not directly related to this ticket: kohadev-XXXX-XX-XX.tar.gz, the file dump, doesn't have the right unix group.

And both .gz files (file dump and db dump) don't have the right group once extracted. Not sure if that's an issue.
Comment 35 Kyle M Hall 2020-12-04 14:04:10 UTC
Created attachment 114195 [details] [review]
Bug 26947: Add scheam-only option to koha-dump

This patch adds a new --schema-only optoin to the koha-dump script.

This can be used to generate a new kohastructure.sql file during the release, and later to
compare the DB structure of a koha instance to the kohastructure.sql file shipped with a version
to identify any missing constraints or other db structure issues

To test:
1 - debian/scripts/koha-dump kohadev
2 - Confirm db and configs are dumped correctly
3 - Apply patch
4 - debian/scripts/koha-dump --help
5 - Confirm new option is listed and makes sense
6 - debian/scripts/koha-dump --schema-only kohadev
7 - Confirm only schema is dumped and is not zipped
8 - debian/scripts/koha-dump kohadev
9 - Confirm entire db is dumped as before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Kyle M Hall 2020-12-04 14:04:23 UTC
Created attachment 114196 [details] [review]
Bug 26947: (follow-up) Set key checks to disabled for kohastructure

This patch adds sql calls to disable foreign key checks for the initial
kohastructure schema load and then re-enables them again for subsequent
actions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Kyle M Hall 2020-12-04 14:04:31 UTC
Created attachment 114197 [details] [review]
Bug 26947: Move -- comment to the COMMENT clause

Sed command from bug 26947 comment 8
sed -r --in-place 's/,\s*-- (.*)$/ COMMENT "\1",/g;' installer/data/mysql/kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 38 Kyle M Hall 2020-12-04 14:04:34 UTC
Created attachment 114198 [details] [review]
Bug 26947: Replace double quotes from comments

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Kyle M Hall 2020-12-04 14:04:38 UTC
Created attachment 114199 [details] [review]
Bug 26947: Correct path for chown and chmod

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Jonathan Druart 2020-12-04 14:40:11 UTC
Created attachment 114201 [details] [review]
Bug 26947: Update kohastructure.sql
Comment 41 David Cook 2020-12-07 01:03:11 UTC
I haven't followed this change line by line, but does this mean that developers will no longer have to update kohastructure.sql? We'll just have to provide atomic updates?
Comment 42 Jonathan Druart 2020-12-09 09:54:38 UTC
(In reply to David Cook from comment #41)
> I haven't followed this change line by line, but does this mean that
> developers will no longer have to update kohastructure.sql? We'll just have
> to provide atomic updates?

Changes to kohastructure.sql are still needed.
Comment 43 David Cook 2020-12-09 23:16:39 UTC
(In reply to Jonathan Druart from comment #42)
>
> Changes to kohastructure.sql are still needed.

Ah, so is this change just about synchronizing kohastructure.sql with the output of a schema only koha-dump? Mostly just so RMs can do a sanity check?
Comment 44 Jonathan Druart 2020-12-10 09:16:17 UTC
I think Nick explained quite well the original need in comment 0:

> Currently the file is out of order, and the conventions for punctuation and 
> formatting are not standard. For instance, some numbers are quoted, some
> tables names are in backticks, some foreign keys are listed as 'CONSTRAINT'
> and some aren't
> 
> The advantage to regenerating the file is that then users can dump their db
> schema and easily diff it against the expected structure to see what might
> be missing.
> 
> Many DB updates simply don't add a new constraint if the existing data
> violates this, and as we use more Koha Objects, these missing constraints
> cause issues.

So it's a need for both standardisation/homogeneity and maintenance.
Comment 45 Jonathan Druart 2021-01-12 10:53:52 UTC
Created attachment 115065 [details] [review]
Bug 26947: Add schema-only option to koha-dump

This patch adds a new --schema-only optoin to the koha-dump script.

This can be used to generate a new kohastructure.sql file during the release, and later to
compare the DB structure of a koha instance to the kohastructure.sql file shipped with a version
to identify any missing constraints or other db structure issues

To test:
1 - debian/scripts/koha-dump kohadev
2 - Confirm db and configs are dumped correctly
3 - Apply patch
4 - debian/scripts/koha-dump --help
5 - Confirm new option is listed and makes sense
6 - debian/scripts/koha-dump --schema-only kohadev
7 - Confirm only schema is dumped and is not zipped
8 - debian/scripts/koha-dump kohadev
9 - Confirm entire db is dumped as before

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Jonathan Druart 2021-01-12 10:53:55 UTC
Created attachment 115066 [details] [review]
Bug 26947: (follow-up) Set key checks to disabled for kohastructure

This patch adds sql calls to disable foreign key checks for the initial
kohastructure schema load and then re-enables them again for subsequent
actions.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Jonathan Druart 2021-01-12 10:54:00 UTC
Created attachment 115067 [details] [review]
Bug 26947: Move -- comment to the COMMENT clause

Sed command from bug 26947 comment 8
sed -r --in-place 's/,\s*-- (.*)$/ COMMENT "\1",/g;' installer/data/mysql/kohastructure.sql

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 48 Jonathan Druart 2021-01-12 10:54:03 UTC
Created attachment 115068 [details] [review]
Bug 26947: Replace double quotes from comments

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 49 Jonathan Druart 2021-01-12 10:54:08 UTC
Created attachment 115069 [details] [review]
Bug 26947: Correct path for chown and chmod

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 50 Jonathan Druart 2021-01-12 10:54:12 UTC
Created attachment 115070 [details] [review]
Bug 26947: Update kohastructure.sql
Comment 51 Jonathan Druart 2021-01-12 11:09:39 UTC
Hum, I was going to push it and found:

% diff /var/spool/koha/kohadev/kohadev-schema-2021-01-12.sql installer/data/mysql/kohastructure.sql                
1c1                                                                                                                                                    
< -- MySQL dump 10.16  Distrib 10.1.47-MariaDB, for debian-linux-gnu (x86_64)                                                                                
---                                                                                                                                                        
> -- MariaDB dump 10.18  Distrib 10.5.8-MariaDB, for debian-linux-gnu (x86_64)                                                                             
5c5                                                                                                                                            
< -- Server version     10.1.48-MariaDB-1~bionic                                                                                      
---                                                                                                                                                
> -- Server version     10.3.27-MariaDB-1:10.3.27+maria~focal                                                                            
28,31c28,31                                                                                                                                           
<   `can_be_added_manually` tinyint(4) NOT NULL DEFAULT '1',                                                                                               
<   `credit_number_enabled` tinyint(1) NOT NULL DEFAULT '0' COMMENT 'Is autogeneration of credit number enabled for this credit type',                         
<   `is_system` tinyint(1) NOT NULL DEFAULT '0',                                                                                                                
<   `archived` tinyint(1) NOT NULL DEFAULT '0' COMMENT 'boolean flag to denote if this till is archived or not',                                 
---                                                                                                                                            
>   `can_be_added_manually` tinyint(4) NOT NULL DEFAULT 1,                                                                                         
>   `credit_number_enabled` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'Is autogeneration of credit number enabled for this credit type',                     
>   `is_system` tinyint(1) NOT NULL DEFAULT 0,                                                                                                        
>   `archived` tinyint(1) NOT NULL DEFAULT 0 COMMENT 'boolean flag to denote if this till is archived or not',   
[SKIP]

% diff /var/spool/koha/kohadev/kohadev-schema-2021-01-12.sql installer/data/mysql/kohastructure.sql | wc -l
1970

Default integer values got quoted in this version of MariaDB (10.1.48-MariaDB-1~bionic), but wasn't in newer version (10.3.27-MariaDB-1:10.3.27+maria~focal)
Comment 52 Jonathan Druart 2021-01-12 11:31:10 UTC
Created attachment 115073 [details] [review]
Bug 26947: Remove AUTO_INCREMENT

Sending a cake there
https://bugs.mysql.com/bug.php?id=20786
Comment 53 Jonathan Druart 2021-01-12 11:32:54 UTC
(In reply to Jonathan Druart from comment #51)
> Default integer values got quoted in this version of MariaDB
> (10.1.48-MariaDB-1~bionic), but wasn't in newer version
> (10.3.27-MariaDB-1:10.3.27+maria~focal)

See https://jira.mariadb.org/browse/MDEV-15377

Deciding to push anyway, it will still be easier to compare and is going to be fixed with higher versions.
Comment 54 Jonathan Druart 2021-01-12 11:34:15 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 55 Fridolin Somers 2021-01-18 14:30:14 UTC
Oh big changes in a very important file.
Is this safe for backport to stable branches ?
Comment 56 Jonathan Druart 2021-01-18 14:50:01 UTC
(In reply to Fridolin Somers from comment #55)
> Oh big changes in a very important file.
> Is this safe for backport to stable branches ?

No, I would not backport it.
Comment 57 Fridolin Somers 2021-01-20 07:26:56 UTC
(In reply to Jonathan Druart from comment #56)
> (In reply to Fridolin Somers from comment #55)
> > Oh big changes in a very important file.
> > Is this safe for backport to stable branches ?
> 
> No, I would not backport it.

Ok no backport :)
Comment 58 Marcel de Rooy 2022-04-07 13:57:58 UTC
Reading this back while looking at changes between structure and actual tables, I am not so sure whether this patch (Move -- to COMMENT) was such a good idea. Note that this is an understatement.

When you add a COMMENT to structure, you should add a db revision too with that clause for that field. Note that DBIx schema files also include those comments.

Since this patch set only touched structure and did not add dbrev's, we will have as a result a very large set of differences between DBIx schema and the tables.
I have been working on resolving differences for a 20.11 database, but I am now already looking forward to the massive output of the dbix script when I upgrade to 21.05 or higher ;)

Note that running dbix script to get those differences would be very useful but will be hugely obscured by the move in this patch set.

What can be done about it? Remove the COMMENT clauses again (revert) ? Adjust dbix script to remove comments (feels like a hack) ?
Comment 59 Marcel de Rooy 2022-04-08 07:37:27 UTC
There are a few DBIx loader options that could be considered:

generate_pod => 0 This seems to be overkill, too rigorous
pod_comment_mode => switching between NAME and DESCRIPTION, not what we need
table_comments_table and column_comments_table => adding those tables would make DBIx look first there instead of the COMMENT in the data source
Comment 60 Marcel de Rooy 2022-04-26 11:18:21 UTC
Ha, stumbled over this report again now.
Why does 20.11 not accept my change in kohastructure, and why does master do?

commit affa3164eb475ff09654c690c9df810be8dc670f
Author: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Date:   Wed Nov 18 17:08:11 2020 +0000
    Bug 26947: (follow-up) Set key checks to disabled for kohastructure

+    # Disable checks before load
+    $self->{'dbh'}->do(q{SET NAMES utf8mb4});
+    $self->{'dbh'}->do(q{SET @OLD_UNIQUE_CHECKS=@@UNIQUE_CHECKS, UNIQUE_CHECKS=0});
+    $self->{'dbh'}->do(q{SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0});
+    $self->{'dbh'}->do(q{SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO'});
+    $self->{'dbh'}->do(q{SET @OLD_SQL_NOTES=@@SQL_NOTES, SQL_NOTES=0});
Comment 61 Marcel de Rooy 2022-04-26 11:21:12 UTC
This latter change could have been backported since it actually resolves a problem that existed in Koha since we introduced DBIx::RunSQL.
DBIx::RunSQL just silently ignore the first lines in kohastructure.sql that are dependent on version:

/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
/*!40101 SET @OLD_CHARACTER_SET_RESULTS=@@CHARACTER_SET_RESULTS */;
/*!40101 SET @OLD_COLLATION_CONNECTION=@@COLLATION_CONNECTION */;
/*!40101 SET NAMES utf8mb4 */;
/*!40103 SET @OLD_TIME_ZONE=@@TIME_ZONE */;
/*!40103 SET TIME_ZONE='+00:00' */;
/*!40014 SET @OLD_UNIQUE_CHECKS=@@UNIQUE_CHECKS, UNIQUE_CHECKS=0 */;
/*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 */;
/*!40101 SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO' */;
/*!40111 SET @OLD_SQL_NOTES=@@SQL_NOTES, SQL_NOTES=0 */;

I wonder if we should remove them from this sql file now, since they are misleading! They are never executed.
Comment 62 Marcel de Rooy 2022-04-26 11:51:39 UTC
(In reply to Marcel de Rooy from comment #61)
> This latter change could have been backported since it actually resolves a
> problem that existed in Koha since we introduced DBIx::RunSQL.
> DBIx::RunSQL just silently ignore the first lines in kohastructure.sql that
> are dependent on version:
> 
> /*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
> /*!40101 SET @OLD_CHARACTER_SET_RESULTS=@@CHARACTER_SET_RESULTS */;
> /*!40101 SET @OLD_COLLATION_CONNECTION=@@COLLATION_CONNECTION */;
> /*!40101 SET NAMES utf8mb4 */;
> /*!40103 SET @OLD_TIME_ZONE=@@TIME_ZONE */;
> /*!40103 SET TIME_ZONE='+00:00' */;
> /*!40014 SET @OLD_UNIQUE_CHECKS=@@UNIQUE_CHECKS, UNIQUE_CHECKS=0 */;
> /*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS,
> FOREIGN_KEY_CHECKS=0 */;
> /*!40101 SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO' */;
> /*!40111 SET @OLD_SQL_NOTES=@@SQL_NOTES, SQL_NOTES=0 */;
> 
> I wonder if we should remove them from this sql file now, since they are
> misleading! They are never executed.

Opened bug 30620 to add at least a comment leaving the possibility to run this directly.
Comment 63 Marcel de Rooy 2022-04-26 11:56:36 UTC
And in line with the former comments, the following commit made lots of useless changes in kohastructure:

commit 8c5845ef9ecd92584723cd60a221ecd4710a2516
Author: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Date:   Fri Dec 4 15:39:32 2020 +0100

    Bug 26947: Update kohastructure.sql

The following pattern was added:
-DROP TABLE IF EXISTS `auth_header`;
-CREATE TABLE `auth_header` (
+DROP TABLE IF EXISTS `account_credit_types`;
+/*!40101 SET @saved_cs_client     = @@character_set_client */;
+/*!40101 SET character_set_client = utf8 */;
+CREATE TABLE `account_credit_types` (

But note that if we run the file in the usual way via Installer, these lines with /*! pass DBIx::RunSQL and are just IGNORED ! They are useless !

Could you remove them, Jonathan ?
Comment 64 Marcel de Rooy 2022-04-26 11:57:27 UTC
(In reply to Marcel de Rooy from comment #63)
> And in line with the former comments, the following commit made lots of
> useless changes in kohastructure:
> 
> commit 8c5845ef9ecd92584723cd60a221ecd4710a2516
> Author: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
> Date:   Fri Dec 4 15:39:32 2020 +0100
> 
>     Bug 26947: Update kohastructure.sql
> 
> The following pattern was added:
> -DROP TABLE IF EXISTS `auth_header`;
> -CREATE TABLE `auth_header` (
> +DROP TABLE IF EXISTS `account_credit_types`;
> +/*!40101 SET @saved_cs_client     = @@character_set_client */;
> +/*!40101 SET character_set_client = utf8 */;
> +CREATE TABLE `account_credit_types` (
> 
> But note that if we run the file in the usual way via Installer, these lines
> with /*! pass DBIx::RunSQL and are just IGNORED ! They are useless !
> 
> Could you remove them, Jonathan ?

Ha copied the wrong DROP/CREATE example, but you ll probaly understand it :)
Comment 65 Jonathan Druart 2022-04-28 08:45:11 UTC
(In reply to Marcel de Rooy from comment #63)
> And in line with the former comments, the following commit made lots of
> useless changes in kohastructure:


> Could you remove them, Jonathan ?

As said on another bug report, this kohastructure.sql will be auto generated when 22.05.00 will be released. We should not add manual changes to it (apart from the usual changes). If you want to modify it you should adjust mysqldump's output (which was already not trivial if you read the previous comments). Or sed it and modify koha-dump.
Comment 66 Marcel de Rooy 2022-11-28 11:05:05 UTC
To still catch differences more easily, I will submitting some code on bug 32334 to sync the comments between actual database and schema.