Bug 37000 - Upgrade fails at 23.12.00.044 [Bug 36120]
Summary: Upgrade fails at 23.12.00.044 [Bug 36120]
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: 24.05
Hardware: All All
: P5 - low blocker (vote)
Assignee: Martin Renvoize
QA Contact: Julian Maurice
URL:
Keywords:
Depends on: 36120
Blocks:
  Show dependency treegraph
 
Reported: 2024-05-31 08:08 UTC by Katrin Fischer
Modified: 2024-06-10 15:36 UTC (History)
5 users (show)

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


Attachments
Bug 37000: [Bug 36120 follow-up] Improve reliability of database update (3.68 KB, patch)
2024-06-04 11:35 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 37000: [Bug 36120 follow-up] Improve reliability of database update (3.74 KB, patch)
2024-06-07 08:43 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 37000: (Bug 36120 follow-up) Improve reliability of database update (3.79 KB, patch)
2024-06-07 12:42 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 37000: (follow-up) Add foreign key last (2.09 KB, patch)
2024-06-07 20:53 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2024-05-31 08:08:56 UTC
Upgrade to 23.12.00.044  [17:47:36]: Bug 36120 - Add pickup location to bookings
        Added column 'bookings.pickup_library_id'
        Set existing bookings pickup location to item homebranch
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Cannot change column 'pickup_library_id': used in a foreign key constraint 'bookings_ibfk_4' at /usr/share/koha/lib/C4/Installer.pm line 741
Comment 1 Martin Renvoize 2024-06-04 11:35:17 UTC
Created attachment 167376 [details] [review]
Bug 37000: [Bug 36120 follow-up] Improve reliability of database update

This patch adds a series of fallthroughs to ensure pickup_library_id is
always set prior to adding the NOT NULL constraint.

We initially only looked at items.homebranch but as that's a nullable
field itself, we now look at items.holdingbranch before finally
defaulting to the first available branch in the branches table in the
worst case.
Comment 2 Chris Cormack 2024-06-07 08:43:41 UTC
Created attachment 167563 [details] [review]
Bug 37000: [Bug 36120 follow-up] Improve reliability of database update

This patch adds a series of fallthroughs to ensure pickup_library_id is
always set prior to adding the NOT NULL constraint.

We initially only looked at items.homebranch but as that's a nullable
field itself, we now look at items.holdingbranch before finally
defaulting to the first available branch in the branches table in the
worst case.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 3 Julian Maurice 2024-06-07 12:42:18 UTC
Created attachment 167573 [details] [review]
Bug 37000: (Bug 36120 follow-up) Improve reliability of database update

This patch adds a series of fallthroughs to ensure pickup_library_id is
always set prior to adding the NOT NULL constraint.

We initially only looked at items.homebranch but as that's a nullable
field itself, we now look at items.holdingbranch before finally
defaulting to the first available branch in the branches table in the
worst case.

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 4 Emily Lamancusa 2024-06-07 16:42:19 UTC
Sorry, this doesn't completely fix the issue for production environments using MySQL. If strict_mode is off, MySQL won't allow you to change a nullable column to non-nullable if the column has a foreign key, even if there are no existing null values in the data. (see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35687#c13)

I was able to reproduce the error, even with this patch applied, with the following steps:
1. Shut down KTD
2. Switch to 23.11.x
3. Start up KTD with MySQL-8 (I did this using the built-in alias ku-my8)
4. Inside a kshell, edit /etc/koha/sites/kohadev/koha-conf.xml. Set <strict-sql-modes> to 0.
5. Switch to main and apply patch
6. restart_all
7. updatedatabase
--> observe the original error

Can the foreign key be added at the end of the dbrev, after the NOT NULL constraint is set, instead of the beginning? items.homebranch and items.holdingbranch already have a FK referencing branches.branchcode themselves, so it should be safe to fill in those values and add the new FK after.

Incidentally, if I ran the database update a second time immediately after the first, the database appeared to successfully complete the update to 24.05; however, this dbrev silently did nothing because the pickup_library_id column already exists, and the foreign key never got added.

Will upload a follow-up patch in a bit that I believe will address both of these issues...
Comment 5 Emily Lamancusa 2024-06-07 20:53:53 UTC
Created attachment 167589 [details] [review]
Bug 37000: (follow-up) Add foreign key last

Certain configurations of MySQL will not allow a column to be changed
from nullable to non-nullable if the column has a foreign key constraint.
Add the foreign key constraint last to avoid issues from this.
Comment 6 Emily Lamancusa 2024-06-07 21:07:43 UTC
Hope I'm not overstepping here...I felt that submitting code was clearer than my comment was on its own, and I didn't want to stall the bug at FQA. Setting back to SO to give Martin and Julian a chance to review.
Comment 7 Martin Renvoize 2024-06-10 15:34:31 UTC
Looks great to me, thanks.
Comment 8 Martin Renvoize 2024-06-10 15:36:09 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant