|
Description
David Cook
2023-06-22 00:55:05 UTC
Created attachment 152543 [details] [review] 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 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! (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. Created attachment 152995 [details] [review] 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 This doesn't take into account "atomic updates" for developers though... But that should be easy enough to take into account... (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... 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... (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. Created attachment 152996 [details] [review] 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 Created attachment 152997 [details] [review] Bug 34088: Fix qa script issues Created attachment 152998 [details] [review] 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 Created attachment 154230 [details] [review] 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> Thanks, Sam :) This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first. *** Bug 26596 has been marked as a duplicate of this bug. *** (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? ;) (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 [U+1F605] (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 [U+1F605] Ok, I guess that's valid :D Sorry, missing tests. (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... I should look at working on this again, as the slowness of "updatedatabase.pl" when there's no work to do is so painful... Created attachment 170554 [details] [review] 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> Created attachment 170555 [details] [review] 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> I decided to take this one over the line by adding the unit tests. Pushed for 24.11! Well done everyone, thank you! Caught bug 34088 when reviewing this (not a bug!) (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? (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. It breaks 00-strict.t (complaining on installer/data/mysql/updatedatabase.pl) on U24 (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 $ (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 (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. Nope, empty output. % 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.
(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-exit()-in-BEGIN 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? 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`. Created attachment 171050 [details] [review] Bug 34088: Move the 'needs update' test to a separate script (In reply to Jonathan Druart from comment #38) > Created attachment 171050 [details] [review] [review] > Bug 34088: Move the 'needs update' test to a separate script Patch for discussion, feel free to obsolete. (In reply to Jonathan Druart from comment #38) > Created attachment 171050 [details] [review] [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 } (In reply to Jonathan Druart from comment #37) > 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 performance ? (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 (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. (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! (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. 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. Yes, I will submit a patch in a few minutes I'm getting an error (HTTP 413 Request entity too large) Patch size is 2.3M Should I attach a gzipped version ? (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. 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. (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. Created attachment 174441 [details] [review] Bug 34088: (follow-up) fix env vars in koha-upgrade-schema 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 (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? Created attachment 174443 [details] [review] Bug 34088: (follow-up) fix env vars in koha-upgrade-schema Signed-off-by: Phil Ringnalda <phil@chetcolibrary.org> (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. Picked the "(follow-up) fix env vars in koha-upgrade-schema" for main. Pushed for 24.11! Well done everyone, thank you! |