Bug 32399 - Database update for bug 30483 is failing
Summary: Database update for bug 30483 is failing
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 30483
Blocks:
  Show dependency treegraph
 
Reported: 2022-12-03 22:09 UTC by Katrin Fischer
Modified: 2023-12-28 20:43 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.01,22.05.12


Attachments
Bug 32399: Unset foreign key checks while changing issues constraint (2.31 KB, patch)
2022-12-05 09:55 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32399: Remove FK constraints while changing columns (3.35 KB, patch)
2022-12-06 14:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 32399: Remove FK constraints while changing columns (3.38 KB, patch)
2022-12-09 07:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 32399: Remove FK constraints while changing columns (3.43 KB, patch)
2022-12-15 13:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 32399: [22.05.x] Remove FK constraints while changing columns (2.76 KB, patch)
2023-04-06 14:35 UTC, Lucas Gass
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2022-12-03 22:09:58 UTC
We have had 2 reports recently of this database update failing:

22.06.00.041  [09:45:46]: Bug 30483 - Make issues.borrowernumber and issues.itemnumber NOT NULLERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'borrowernumber': used in a foreign key constraint 'issues_ibfk_1' at /usr/share/koha/lib/C4/Installer.pm line 739

And:

Upgrade to 22.05.06.001 [19:37:23]: Bug 30483 - Make issues.borrowernumber and itemnumber NOT NULL
ERROR: {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'borrowernumber': used in a foreign key constraint 'issues_ibfk_1' at /usr/share/koha/lib/C4/Installer.pm line 739 

Both refer to the same bug/SQL statements, but backported to different versions.
Comment 1 Katrin Fischer 2022-12-04 11:21:53 UTC
While we first thought this to be a data issue, it might not be the case, from the mailing list:

Nope. In fact, issues has only one row, with its integer on borrowernumber:
issue_id: 2. borrowernumber: 53805. issuer_id: NULL. itemnumber: 1581. date_due: 2020-10-08 23:59:00. branchcode: BC. returndate: NULL. lastreneweddate: NULL. renewals: 0. unseen_renewals: 0. auto_renew: 0. auto_renew_error: NULL. timestamp: 2020-10-07 15:29:52. issuedate: 2020-10-07 

borrowernumber (and itemnumber) are not NULL in the only row of issues table. Also, the borrowernumber (53805) exists on borrowers table. Could be something else?
Comment 2 Marcel de Rooy 2022-12-05 07:46:52 UTC
Could you provide details on the exact SQL version used here ?
Comment 3 Marcel de Rooy 2022-12-05 09:55:47 UTC
Created attachment 144428 [details] [review]
Bug 32399: Unset foreign key checks while changing issues constraint

Some users have reported that the dbrev of bug 30483 fails. This might
depend on bad data, SQL server version, etc.
The easiest workaround is now to unset the check temporarily.
Maybe we should also warn for issues without itemnumber or
borrowernumber on About? Or remove them?

Test plan:
Reset your version in order to pass this dbrev again. So below
22.06.00.041.
* update systempreferences set value='22.0600040' where variable = 'Version';
Run updatedatabase.

Bonus: Create an issues record with invalid itemnumber (set FK check
off when doing so). Verify that dbrev prints warning about it.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2022-12-05 09:59:13 UTC
This needs a backport to 22.11 and 22.05
Comment 5 Pablo AB 2022-12-05 11:41:36 UTC
(In reply to Marcel de Rooy from comment #2)
> Could you provide details on the exact SQL version used here ?

innodb_version           5.6.44-86.0
protocol_version         10
version                  10.1.41-MariaDB-0+deb9u1
version_comment          Debian 9.9
version_compile_machine  x86_64
version_compile_os       debian-linux-gnu
version_malloc_library   system jemalloc
version_ssl_library      YaSSL 2.4.4
wsrep_patch_version      wsrep_25.24

Debian 10 (lsb_release -a)
koha-common    22.05.07-1 (upgrading from 20.05)
Comment 6 Marcel de Rooy 2022-12-05 11:55:41 UTC
(In reply to Pablo AB from comment #5)
> 
> innodb_version           5.6.44-86.0
> protocol_version         10
> version                  10.1.41-MariaDB-0+deb9u1
> version_comment          Debian 9.9
> version_compile_machine  x86_64
> version_compile_os       debian-linux-gnu
> version_malloc_library   system jemalloc
> version_ssl_library      YaSSL 2.4.4
> wsrep_patch_version      wsrep_25.24
> 
> Debian 10 (lsb_release -a)
> koha-common    22.05.07-1 (upgrading from 20.05)

Ah, thanks.
We seem to bump on SQL version differences. I tested with MariaDB 10.4/10.5 without the issues you reported. But 10.1 apparently has.
The attached patch should address it.
Comment 7 Jonathan Druart 2022-12-05 13:45:29 UTC
I don't think we should turn FOREIGN_KEY_CHECKS off. IMO we should add the warning and stop the upgrade process, wait for them to fix the data, then continue the upgrade.
Comment 8 Tomás Cohen Arazi 2022-12-05 14:07:00 UTC
(In reply to Jonathan Druart from comment #7)
> I don't think we should turn FOREIGN_KEY_CHECKS off. IMO we should add the
> warning and stop the upgrade process, wait for them to fix the data, then
> continue the upgrade.

I agree. We should definitely give a better error message.
Comment 9 Marcel de Rooy 2022-12-05 14:28:26 UTC
Yes, but could this be sql server/version specific ?
Comment 10 Marcel de Rooy 2022-12-05 14:29:03 UTC
As reported, there should not have been bad data ??
Comment 11 Katrin Fischer 2022-12-05 15:05:11 UTC
(In reply to Marcel de Rooy from comment #10)
> As reported, there should not have been bad data ??

yes, we had a good report on it not being the data.
Comment 12 Marcel de Rooy 2022-12-06 14:04:48 UTC
Created attachment 144448 [details] [review]
Bug 32399: Remove FK constraints while changing columns

Some users have reported that the dbrev of bug 30483 fails. This might
depend on bad data, SQL server version, etc.
We now remove the constraints temporarily.

Test plan:
Reset your version in order to pass this dbrev again. So below
22.06.00.041.
* update systempreferences set value='22.0600040' where variable = 'Version';
Run updatedatabase.

Bonus: Create an issues record with invalid itemnumber (set FK check
off when doing so). Verify that dbrev fails and installer stops.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2022-12-06 14:05:37 UTC
Plan B activated: remove constraints temporarily. Bail out when we have bad data.
Comment 14 Marcel de Rooy 2022-12-06 14:07:10 UTC
Observation from testing:
The step3.tt template sticks the error to each line of the output. When you want to print 4 lines, this is kind of silly.
Not solving this here.
If you like, open a new report :)
Comment 15 Martin Renvoize 2022-12-09 07:35:40 UTC
Created attachment 144516 [details] [review]
Bug 32399: Remove FK constraints while changing columns

Some users have reported that the dbrev of bug 30483 fails. This might
depend on bad data, SQL server version, etc.
We now remove the constraints temporarily.

Test plan:
Reset your version in order to pass this dbrev again. So below
22.06.00.041.
* update systempreferences set value='22.0600040' where variable = 'Version';
Run updatedatabase.

Bonus: Create an issues record with invalid itemnumber (set FK check
off when doing so). Verify that dbrev fails and installer stops.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-12-09 07:37:34 UTC
This is a real improvement in my opinion.. signing off.
Comment 17 Tomás Cohen Arazi 2022-12-15 13:41:11 UTC
Created attachment 144616 [details] [review]
Bug 32399: Remove FK constraints while changing columns

Some users have reported that the dbrev of bug 30483 fails. This might
depend on bad data, SQL server version, etc.
We now remove the constraints temporarily.

Test plan:
Reset your version in order to pass this dbrev again. So below
22.06.00.041.
* update systempreferences set value='22.0600040' where variable = 'Version';
Run updatedatabase.

Bonus: Create an issues record with invalid itemnumber (set FK check
off when doing so). Verify that dbrev fails and installer stops.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 18 Tomás Cohen Arazi 2022-12-15 14:27:59 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 19 Marcel de Rooy 2022-12-15 14:43:46 UTC
RMaints: We need this in 22.11 and 22.05. Please let me know if you need assistance.
Comment 20 Martin Renvoize 2022-12-18 08:22:23 UTC
Nice work everyone!

Pushed to 22.11.x for the next release
Comment 21 Marcel de Rooy 2023-03-21 12:15:29 UTC
Why wasn't this pushed to 22.05 yet ?
Comment 22 Lucas Gass 2023-04-05 20:11:11 UTC
(In reply to Marcel de Rooy from comment #21)
> Why wasn't this pushed to 22.05 yet ?

I'm confused about this patch. It won't apply to 22.05 because the update is directly in installer/data/mysql/atomicupdate/db_revs.

Should it not be in 'installer/data/mysql/atomicupdate/atomicupdate' and the Rmaints move it to db_revs?


Sorry for the confusion.
Comment 23 Marcel de Rooy 2023-04-06 05:41:52 UTC
(In reply to Lucas Gass from comment #22)
> (In reply to Marcel de Rooy from comment #21)
> > Why wasn't this pushed to 22.05 yet ?
> 
> I'm confused about this patch. It won't apply to 22.05 because the update is
> directly in installer/data/mysql/atomicupdate/db_revs.
> 
> Should it not be in 'installer/data/mysql/atomicupdate/atomicupdate' and the
> Rmaints move it to db_revs?
> 
> 
> Sorry for the confusion.

Sure. Good plan! Dont wait any longer. This is a fix for something that was pushed to 22.05.07.
Comment 24 Lucas Gass 2023-04-06 14:35:09 UTC
Created attachment 149235 [details] [review]
Bug 32399: [22.05.x] Remove FK constraints while changing columns
Comment 25 Lucas Gass 2023-04-06 14:36:06 UTC
Marcel,

Please have a look at the patch I added for 22.05.x and conform it is correct. 

Thanks!
Comment 26 Marcel de Rooy 2023-04-07 06:16:16 UTC
(In reply to Lucas Gass from comment #25)
> Marcel,
> 
> Please have a look at the patch I added for 22.05.x and conform it is
> correct. 
> 
> Thanks!

Looks fine to me. Thanks.
Comment 27 Lucas Gass 2023-04-24 15:54:04 UTC
Backported to 22.05.x for upcoming 22.05.12
Comment 28 Arthur Suzuki 2023-05-03 18:34:30 UTC
won't backport, depends on 30483 which is not applied to 21.11.x