Bug 34088 - Schema upgrade should short circuit faster if no upgrade needs to be done
Summary: Schema upgrade should short circuit faster if no upgrade needs to be done
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Cook
QA Contact: Testopia
URL:
Keywords:
: 26596 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-06-22 00:55 UTC by David Cook
Modified: 2023-12-12 23:10 UTC (History)
2 users (show)

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


Attachments
Bug 34088: Short circuit database upgrade check (2.02 KB, patch)
2023-06-22 00:57 UTC, David Cook
Details | Diff | Splinter Review
Bug 34088: Short circuit database upgrade check (3.50 KB, patch)
2023-07-04 00:57 UTC, David Cook
Details | Diff | Splinter Review
Bug 34088: Short circuit database upgrade check (5.66 KB, patch)
2023-07-04 01:53 UTC, David Cook
Details | Diff | Splinter Review
Bug 34088: Fix qa script issues (1.71 KB, patch)
2023-07-04 02:01 UTC, David Cook
Details | Diff | Splinter Review
Bug 34088: Short circuit database upgrade check (5.82 KB, patch)
2023-07-04 02:03 UTC, David Cook
Details | Diff | Splinter Review
Bug 34088: Short circuit database upgrade check (5.86 KB, patch)
2023-08-03 17:57 UTC, Sam Lau
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Cook 2023-06-22 00:55:05 UTC
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.
Comment 1 David Cook 2023-06-22 00:57:02 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.
Comment 2 David Cook 2023-06-22 00:58:19 UTC
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!
Comment 3 David Cook 2023-07-04 00:27:37 UTC Comment hidden (obsolete)
Comment 4 David Cook 2023-07-04 00:57:27 UTC Comment hidden (obsolete)
Comment 5 David Cook 2023-07-04 01:09:56 UTC Comment hidden (obsolete)
Comment 6 David Cook 2023-07-04 01:30:41 UTC Comment hidden (obsolete)
Comment 7 David Cook 2023-07-04 01:43:29 UTC Comment hidden (obsolete)
Comment 8 David Cook 2023-07-04 01:46:55 UTC Comment hidden (obsolete)
Comment 9 David Cook 2023-07-04 01:53:03 UTC Comment hidden (obsolete)
Comment 10 David Cook 2023-07-04 02:01:54 UTC Comment hidden (obsolete)
Comment 11 David Cook 2023-07-04 02:03:24 UTC
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
Comment 12 Sam Lau 2023-08-03 17:57:18 UTC
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>
Comment 13 David Cook 2023-08-04 01:08:40 UTC
Thanks, Sam :)
Comment 14 David Cook 2023-09-06 05:55:16 UTC
This one is a nice performance boost. I'm tempted to implement it locally, but I'd love to upstream it first.
Comment 15 David Cook 2023-09-12 23:37:29 UTC
*** Bug 26596 has been marked as a duplicate of this bug. ***
Comment 16 Katrin Fischer 2023-11-05 13:20:42 UTC
(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? ;)
Comment 17 David Cook 2023-11-05 22:16:37 UTC
(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 😅
Comment 18 Katrin Fischer 2023-11-06 10:00:37 UTC
(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
Comment 19 Jonathan Druart 2023-12-12 08:11:39 UTC
Sorry, missing tests.
Comment 20 David Cook 2023-12-12 23:10:17 UTC
(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...