Summary: | Database update for bug 30483 is failing | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | Installation and upgrade (web-based installer) | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | major | ||
Priority: | P5 - low | CC: | arthur.suzuki, gmcharlt, jonathan.druart, lucas, m.de.rooy, martin.renvoize, pablo.bianchi, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
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
|
|
Circulation function: | |||
Bug Depends on: | 30483 | ||
Bug Blocks: | |||
Attachments: |
Bug 32399: Unset foreign key checks while changing issues constraint
Bug 32399: Remove FK constraints while changing columns Bug 32399: Remove FK constraints while changing columns Bug 32399: Remove FK constraints while changing columns Bug 32399: [22.05.x] Remove FK constraints while changing columns |
Description
Katrin Fischer
2022-12-03 22:09:58 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? Could you provide details on the exact SQL version used here ? 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> This needs a backport to 22.11 and 22.05 (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) (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. 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. (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. Yes, but could this be sql server/version specific ? As reported, there should not have been bad data ?? (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. 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> Plan B activated: remove constraints temporarily. Bail out when we have bad data. 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 :) 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> This is a real improvement in my opinion.. signing off. 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> Pushed to master for 23.05. Nice work everyone, thanks! RMaints: We need this in 22.11 and 22.05. Please let me know if you need assistance. Nice work everyone! Pushed to 22.11.x for the next release Why wasn't this pushed to 22.05 yet ? (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. (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. Created attachment 149235 [details] [review] Bug 32399: [22.05.x] Remove FK constraints while changing columns Marcel, Please have a look at the patch I added for 22.05.x and conform it is correct. Thanks! (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. Backported to 22.05.x for upcoming 22.05.12 won't backport, depends on 30483 which is not applied to 21.11.x |