[Bug 34088] New: Schema upgrade should short circuit faster if no upgrade needs to be done
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Bug ID: 34088 Summary: Schema upgrade should short circuit faster if no upgrade needs to be done Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: dcook@prosentient.com.au QA Contact: testopia@bugs.koha-community.org At the moment, if there is no database upgrade to run, updatedatabase.pl can take over 1.1 seconds to run. If you chain together 60 runs of updatedatabase.pl via koha-upgrade-schema and "apt-get install koha-common", that takes well over 1 minute at best to check that nothing needs to be done. (In practice, I think it goes over 1 minute.) We should be short circuiting earlier because we have the code version at hand and it's easy to check the database version. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- Created attachment 152543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152543&action=edit Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |dcook@prosentient.com.au |ity.org | --- Comment #2 from David Cook <dcook@prosentient.com.au> --- This isn't quite ready for NSO, as it needs to be polished, but I need to switch tasks right now, and I didn't want to forget to raise the ticket. This works very well. It takes about .06 seconds to run koha-upgrade-schema instead of 1.1 seconds. If you multiply that by 60, that is 3.6 seconds instead of 66 seconds. Huge difference! -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=27880 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #2)
This isn't quite ready for NSO, as it needs to be polished
What I mean by this is just that other code in Koha should rely on Koha::Installer::TransformToNum. At the moment, Koha::Installer::TransformToNum is copy/pasted from C4::Installer, which is obviously not OK. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152543|0 |1 is obsolete| | --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Created attachment 152995 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152995&action=edit Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if an update is needed (based off database version and code version). It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test C4::Installer, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #5 from David Cook <dcook@prosentient.com.au> --- This doesn't take into account "atomic updates" for developers though... But that should be easy enough to take into account... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #5)
This doesn't take into account "atomic updates" for developers though...
But that should be easy enough to take into account...
Hmm taking "atomic updates" into account makes it significantly slower as the "require C4::Context" alone adds about .25 seconds, which becomes significant when you multiply it against higher numbers... But "require Koha::Config" is very fast... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- So I've just made a wild discovery... I'm adding a function to determine whether or not there are any atomic updates. The check is actually very quick, but when you return a non-null value - even if it's an empty arrayref, the time slows down by 1.152 seconds. It's bizarre... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #8 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #7)
So I've just made a wild discovery...
I'm adding a function to determine whether or not there are any atomic updates. The check is actually very quick, but when you return a non-null value - even if it's an empty arrayref, the time slows down by 1.152 seconds.
It's bizarre...
Ah wait(In reply to David Cook from comment #7)
So I've just made a wild discovery...
I'm adding a function to determine whether or not there are any atomic updates. The check is actually very quick, but when you return a non-null value - even if it's an empty arrayref, the time slows down by 1.152 seconds.
It's bizarre...
Ah wait it was obvious in the end. If something seems magical, you're clearly just missing something obvious hehe. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152995|0 |1 is obsolete| | --- Comment #9 from David Cook <dcook@prosentient.com.au> --- Created attachment 152996 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152996&action=edit Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #10 from David Cook <dcook@prosentient.com.au> --- Created attachment 152997 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152997&action=edit Bug 34088: Fix qa script issues -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152996|0 |1 is obsolete| | Attachment #152997|0 |1 is obsolete| | --- Comment #11 from David Cook <dcook@prosentient.com.au> --- Created attachment 152998 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=152998&action=edit Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Sam Lau <samalau@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Sam Lau <samalau@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #152998|0 |1 is obsolete| | --- Comment #12 from Sam Lau <samalau@gmail.com> --- Created attachment 154230 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=154230&action=edit Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted Signed-off-by: Sam Lau <samalau@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- Thanks, Sam :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #14 from David Cook <dcook@prosentient.com.au> --- This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #15 from David Cook <dcook@prosentient.com.au> --- *** Bug 26596 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Blou <philippe.blouin@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |philippe.blouin@inlibro.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to David Cook from comment #14)
This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first.
Maybe QA it? ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #17 from David Cook <dcook@prosentient.com.au> --- (In reply to Katrin Fischer from comment #16)
(In reply to David Cook from comment #14)
This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first.
Maybe QA it? ;)
I wrote it so I can't QA it 😅 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to David Cook from comment #17)
(In reply to Katrin Fischer from comment #16)
(In reply to David Cook from comment #14)
This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first.
Maybe QA it? ;)
I wrote it so I can't QA it 😅
Ok, I guess that's valid :D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart+koha@gmail. | |com --- Comment #19 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Sorry, missing tests. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #20 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #19)
Sorry, missing tests.
Good call. It looks like they should be fairly easy to add... I should also replace @stuff with something more descriptive... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #21 from David Cook <dcook@prosentient.com.au> --- I should look at working on this again, as the slowness of "updatedatabase.pl" when there's no work to do is so painful... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37657 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #154230|0 |1 is obsolete| | --- Comment #22 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 170554 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170554&action=edit Bug 34088: Short circuit database upgrade check If the database version and the code version are the same, we should short circuit and exit immediately. This patch adds the Koha::Installer module which can very quickly check if a db or atomic update is needed. It also moves the logic for C4::Installer::TransformToNum to Koha::Installer::TransformToNum for performance reasons. It also moves the logic for C4::Installer::get_atomic_updates to Koha::Installer::get_atomic_updates for performance reasons. Test plan: 1. Apply patch 2. Run `time koha-upgrade-schema kohadev` 3. Note that it completes in less than .1 seconds 4. To test db updates, change the database Version to a number slightly behind the code version, and run `time koha-upgrade-schema kohadev` 5. Note that the correct version update is processed 6. To test atomic updates: cp installer/data/mysql/atomicupdate/skeleton.pl \ installer/data/mysql/atomicupdate/bug_34088.pl 7. Run `time koha-upgrade-schema kohadev` 8. Note that it takes over 1 second to run and the atomic update is attempted Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 170555 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=170555&action=edit Bug 34088: Add unit tests This patch adds unit tests to cover the moved and newly introduced methods in the Koha namespace. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #24 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I decided to take this one over the line by adding the unit tests. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.11.00 released in| | Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #25 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=37818 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #26 from Jonathan Druart <jonathan.druart@gmail.com> --- Caught bug 34088 when reviewing this (not a bug!) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #27 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #26)
Caught bug 34088 when reviewing this (not a bug!)
I'm sorry but I don't understand this comment. Could you re-phrase? Is that the right bug reference? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #28 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #27)
(In reply to Jonathan Druart from comment #26)
Caught bug 34088 when reviewing this (not a bug!)
I'm sorry but I don't understand this comment. Could you re-phrase? Is that the right bug reference?
Sorry, wrong number, it is bug 37818. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed --- Comment #29 from Jonathan Druart <jonathan.druart@gmail.com> --- It breaks 00-strict.t (complaining on installer/data/mysql/updatedatabase.pl) on U24 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #30 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #29)
It breaks 00-strict.t (complaining on installer/data/mysql/updatedatabase.pl) on U24
Using KOHA_IMAGE=main (perl v5.36.0) $ perl -wc installer/data/mysql/updatedatabase.pl installer/data/mysql/updatedatabase.pl syntax OK $ KOHA_IMAGE=main-noble (perl v5.38.2) $ perl -wc installer/data/mysql/updatedatabase.pl $ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #31 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #29)
It breaks 00-strict.t (complaining on installer/data/mysql/updatedatabase.pl) on U24
Koha_Main_U24/73 https://jenkins.koha-community.org/job/Koha_Main_U24/73/consoleFull 07:44:08 koha_1 | # Failed test 'Syntax check installer/data/mysql/updatedatabase.pl' 07:44:08 koha_1 | # at /usr/share/perl5/Test/Strict.pm line 435. 07:44:08 koha_1 | # 07:44:08 koha_1 | # Looks like you failed 1 test of 1183. 07:44:08 koha_1 | [05:40:16] t/db_dependent/00-strict.t 07:44:08 koha_1 | Dubious, test returned 1 (wstat 256, 0x100) 07:44:08 koha_1 | Failed 1/1183 subtests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #32 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #30)
(In reply to Jonathan Druart from comment #29)
It breaks 00-strict.t (complaining on installer/data/mysql/updatedatabase.pl) on U24
Using KOHA_IMAGE=main (perl v5.36.0) $ perl -wc installer/data/mysql/updatedatabase.pl installer/data/mysql/updatedatabase.pl syntax OK $
KOHA_IMAGE=main-noble (perl v5.38.2) $ perl -wc installer/data/mysql/updatedatabase.pl $
That's interesting. Can you try this on the U24: perl -Mdiagnostics -wc installer/data/mysql/updatedatabase.pl -- Hopefully that should provide some more useful information. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #33 from Jonathan Druart <jonathan.druart@gmail.com> --- Nope, empty output. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #34 from Jonathan Druart <jonathan.druart@gmail.com> --- % more foo.pl #!/usr/bin/perl use Modern::Perl; BEGIN { say "exit"; exit; }; say "hey!"; On my host, perl v5.34.0 % perl -c foo.pl exit foo.pl syntax OK within ktd noble (perl v5.38.2) root@kohadevbox:koha$ perl -c foo.pl exit The doc states that the begin block will be executed (same in both versions): -c causes Perl to check the syntax of the program and then exit without executing it. Actually, it will execute any BEGIN, UNITCHECK, or CHECK blocks and any use statements: these are considered as occurring outside the execution of your program. INIT and END blocks, however, will be skipped. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #35 from Jonathan Druart <jonathan.druart@gmail.com> --- Commented on https://github.com/Perl/perl5/issues/22143#issuecomment-2330945147 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #36 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Jonathan Druart from comment #35)
Commented on https://github.com/Perl/perl5/issues/22143#issuecomment-2330945147
Looks like you already got a reply, but I am not sure what it means: https://perldoc.perl.org/perl5380delta#INIT-blocks-no-longer-run-after-an-ex... INIT blocks no longer run after an exit() in BEGIN INIT blocks will no longer run after an exit() performed inside of a BEGIN. This means that the combination of the -v option and the -c option no longer executes a compile check as well as showing the perl version. The -v option executes an exit(0) after printing the version information inside of a BEGIN block, and the -c check is implemented by using INIT hooks, resulting in the -v option taking precedence. What can we do to fix the failure? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #37 from Jonathan Druart <jonathan.druart@gmail.com> --- Either add an exception for updatedatabase.pl to 00-strict.t (bad) or we can have a separate script installer/data/mysql/needs_update.pl that does the test, and adjust the updatedatabase.pl calls with `needs_update.pl && updatedatabase.pl`. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #38 from Jonathan Druart <jonathan.druart@gmail.com> --- Created attachment 171050 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=171050&action=edit Bug 34088: Move the 'needs update' test to a separate script -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #39 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #38)
Created attachment 171050 [details] [review] Bug 34088: Move the 'needs update' test to a separate script
Patch for discussion, feel free to obsolete. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #40 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to Jonathan Druart from comment #38)
Created attachment 171050 [details] [review] Bug 34088: Move the 'needs update' test to a separate script
ktd's .bashrc will need to be adjusted (and not fail if needs_update.pl does not exist): 63 # function for running updatedatabase.pl 64 function updatedatabase(){ 65 sudo koha-shell kohadev -p -c 'perl /kohadevbox/koha/installer/data/mysql/updatedatabase.pl' 66 } -- You are receiving this mail because: You are watching all bug changes.
Either add an exception for updatedatabase.pl to 00-strict.t (bad) or we can have a separate script installer/data/mysql/needs_update.pl that does the test, and adjust the updatedatabase.pl calls with `needs_update.pl && updatedatabase.pl`. IIUC the needs_update call is in a BEGIN block to avoid loading all modules if no update is needed. What about 'require'-ing the slow modules instead, so we can move the needs_update call out of the BEGIN block without losing
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #41 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Jonathan Druart from comment #37) performance ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #42 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Julian Maurice from comment #41)
IIUC the needs_update call is in a BEGIN block to avoid loading all modules if no update is needed. What about 'require'-ing the slow modules instead, so we can move the needs_update call out of the BEGIN block without losing performance ?
Nevermind, we'd lose performance anyway (from 36ms to 200ms for me), probably because Perl has to parse the whole file. Another idea, Joubu's idea but reversed: keep only the needs_update call in updatedatabase.pl and move the rest in a new file, updatedatabase.pl would call/eval/do this new file. That way we don't need to update all scripts that rely on updatedatabase.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #43 from David Cook <dcook@prosentient.com.au> --- (In reply to Julian Maurice from comment #42)
Another idea, Joubu's idea but reversed: keep only the needs_update call in updatedatabase.pl and move the rest in a new file, updatedatabase.pl would call/eval/do this new file. That way we don't need to update all scripts that rely on updatedatabase.pl
I like that. I like that a lot. It would be a very light touch while still being effective. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #44 from David Cook <dcook@prosentient.com.au> --- (In reply to David Cook from comment #43)
(In reply to Julian Maurice from comment #42)
Another idea, Joubu's idea but reversed: keep only the needs_update call in updatedatabase.pl and move the rest in a new file, updatedatabase.pl would call/eval/do this new file. That way we don't need to update all scripts that rely on updatedatabase.pl
I like that. I like that a lot. It would be a very light touch while still being effective.
I especially like this, because koha-upgrade-schema isn't the only way to call updatedatabase.pl. There are git installs, tarball installs, et cetera which require manually calling updatedatabase.pl. Moving the bulk of updatedatabase.pl into another file would be the least disruptive. Good one, Jonathan and Julian! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #45 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #44)
(In reply to David Cook from comment #43)
(In reply to Julian Maurice from comment #42)
Another idea, Joubu's idea but reversed: keep only the needs_update call in updatedatabase.pl and move the rest in a new file, updatedatabase.pl would call/eval/do this new file. That way we don't need to update all scripts that rely on updatedatabase.pl
I like that. I like that a lot. It would be a very light touch while still being effective.
I especially like this, because koha-upgrade-schema isn't the only way to call updatedatabase.pl.
I didn't find any other occurrences that needed to be adjusted, hence my version. I am letting you fix it another way then. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #46 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- David or Julian - do you want to provide an alternative? We need Jenkins to go green again, I have halted pushing enhancement and most bugfixes for now. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #47 from Julian Maurice <julian.maurice@biblibre.com> --- Yes, I will submit a patch in a few minutes -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #48 from Julian Maurice <julian.maurice@biblibre.com> --- I'm getting an error (HTTP 413 Request entity too large) Patch size is 2.3M Should I attach a gzipped version ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #49 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (In reply to Julian Maurice from comment #48)
I'm getting an error (HTTP 413 Request entity too large) Patch size is 2.3M Should I attach a gzipped version ?
Can you push the branch/patches to a public repository? Much easier to handle with git and tools. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #50 from Julian Maurice <julian.maurice@biblibre.com> --- https://gitlab.com/jajm/Koha/-/commit/1e12a54e42de196df07a6e8223f19a3e194407... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #51 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi everyone, first of all thanks for the work you put into resolving this. After some back and forth and discussion I have decided to push Jonathan's patch which is the smaller change in a way. Let's hope Jenkins agrees. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |release-notes-needed -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #52 from David Cook <dcook@prosentient.com.au> --- (In reply to Katrin Fischer from comment #51)
Hi everyone, first of all thanks for the work you put into resolving this. After some back and forth and discussion I have decided to push Jonathan's patch which is the smaller change in a way. Let's hope Jenkins agrees.
Looks like I missed this conversation as I was overseas and didn't see the email when I was catching up after Kohacon. At a glance, the change looks good. It doesn't address some of my earlier concerns, but practically speaking koha-upgrade-schema is what needs the fix, so this makes sense. -- However, was this actually tested? Because it doesn't work. No update needed works: sudo ./debian/scripts/koha-upgrade-schema kohadev testa Upgrading database schema for kohadev Upgrading database schema for testa Update needed does not work: sudo ./debian/scripts/koha-upgrade-schema kohadev testa Upgrading database schema for kohadev Can't locate C4/Context.pm in @INC (you may need to install the C4::Context module) (@INC contains: /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.36.0 /usr/local/share/perl/5.36.0 /usr/lib/x86_64-linux-gnu/perl5/5.36 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl-base /usr/lib/x86_64-linux-gnu/perl/5.36 /usr/share/perl/5.36 /usr/local/lib/site_perl) at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 39. BEGIN failed--compilation aborted at /kohadevbox/koha/installer/data/mysql/updatedatabase.pl line 39. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #53 from David Cook <dcook@prosentient.com.au> --- Created attachment 174441 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174441&action=edit Bug 34088: (follow-up) fix env vars in koha-upgrade-schema -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #54 from David Cook <dcook@prosentient.com.au> --- While this is already pushed, I think that attaching the patch here is the right thing? Let me know if you need it attached elsewhere. To prove/test the problem/solution of this last patch: 1. Look up your DB version e.g. select value from systempreferences where variable = 'Version'; 2. Set the DB version to 1 version behind the code version e.g. update systempreferences set value = '24.0600056' where variable = 'Version'; 3. Run DB update e.g. sudo ./debian/scripts/koha-upgrade-schema kohadev 4. Note no errors and database successfully upgrades -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #55 from David Cook <dcook@prosentient.com.au> --- (In reply to Katrin Fischer from comment #51)
Hi everyone, first of all thanks for the work you put into resolving this. After some back and forth and discussion I have decided to push Jonathan's patch which is the smaller change in a way. Let's hope Jenkins agrees.
If this hasn't been impacting Jenkins, I'm guessing that the environmental variables are already set int he background for 1 particular Jenkins instance, and that's why it wasn't causing problems? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|release-notes-needed | Text to go in the| |This change makes release notes| |koha-schema-upgrade use an | |optimized check if a | |database upgrade is needed | |before attempting the usual | |slower upgrade process. | |This makes Koha upgrades, | |which don't need database | |updates, much faster. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #174441|0 |1 is obsolete| | --- Comment #56 from Phil Ringnalda <phil@chetcolibrary.org> --- Created attachment 174443 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=174443&action=edit Bug 34088: (follow-up) fix env vars in koha-upgrade-schema Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Phil Ringnalda <phil@chetcolibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |phil@chetcolibrary.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #57 from Jonathan Druart <jonathan.druart@gmail.com> --- (In reply to David Cook from comment #55)
(In reply to Katrin Fischer from comment #51)
Hi everyone, first of all thanks for the work you put into resolving this. After some back and forth and discussion I have decided to push Jonathan's patch which is the smaller change in a way. Let's hope Jenkins agrees.
If this hasn't been impacting Jenkins, I'm guessing that the environmental variables are already set int he background for 1 particular Jenkins instance, and that's why it wasn't causing problems?
Jenkins is not testing the upgrade via the debian package scripts. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #58 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Picked the "(follow-up) fix env vars in koha-upgrade-schema" for main. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #59 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.11! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED CC| |lucas@bywatersolutions.com Status|Pushed to main |RESOLVED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Architecture, internals, |Installation and upgrade |and plumbing |(web-based installer) CC| |gmcharlt@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 Martin Renvoize (ashimema) <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Installation and upgrade |Installation and upgrade |(web-based installer) |(command-line installer) CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |39460 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=39460 [Bug 39460] Debian package install broken in 24.11 if no database change included in package -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34088 --- Comment #60 from David Cook <dcook@prosentient.com.au> --- It looks like the use of "needs_update.pl" causes a regression in koha-upgrade-schema for certain install scenarios. See bug 39460. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org