Bug 23377 - bulkmarcimport.pl disables syspref caching
Summary: bulkmarcimport.pl disables syspref caching
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor
Assignee: Ian Walls
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-07-25 20:04 UTC by Ian Walls
Modified: 2021-06-14 21:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.03, 19.05.08
Circulation function:


Attachments
Bug 23377: use ENV syspref override instead of complete disable (2.79 KB, patch)
2019-07-31 16:55 UTC, Ian Walls
Details | Diff | Splinter Review
Bug 23377: use ENV syspref override instead of complete disable (2.84 KB, patch)
2020-01-09 22:07 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 23377: use ENV syspref override instead of complete disable (2.93 KB, patch)
2020-01-10 10:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23377: (QA follow-up) Use OVERRIDE_SYSPREF (1.48 KB, patch)
2020-01-10 10:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ian Walls 2019-07-25 20:04:41 UTC
bulkmarcimport.pl disables syspref caching in order to explicitly set CataloguingLog and AuthoritiesLog to false (in order to boost performance, according to the notes).  However, doing so results in numerous other syspref lookups over the course of the script, increasing runtime by about 33%.  Sysprefs looked up during a plain, no-options import include:

1. DefaultClassificationSource    
2. TimeFormat
3. dateformat
4. CataloguingLog
5. BiblioAddsAuthorities
6. marcflavour
7. SearchEngine
8. OAI-PMH:AutoUpdateSets

While disabling additional logging on import is a good thing for performance, these additional DB queries offset that gain.  Using the ENV variable override option may be a better way to temporarily disable these logs for the course of the script, whilst still keeping all the other sysprefs cached.
Comment 1 Ian Walls 2019-07-31 16:55:18 UTC
Created attachment 91898 [details] [review]
Bug 23377: use ENV syspref override instead of complete disable

In an attempt to save time, bulkmarcimport temporarily sets CataloguingLog
and AuthoritiesLog to 0.  It does this by disabling syspref caching and saving
the changes to the database (then replacing the original values at completion).

Unfortunately, this disables other key sysprefs from being cached, and results in
a 50% increase in processing time for the script.

This patch instead utilizes the ENV variable override feature of sysprefs, which
preempts the cache in C4::Context->preference().

To test:
1. Perform a bulkmarcimport with a reasonable number of biblios (~1000 will do)
2. Note the time taken to complete
3. Apply patch
4. Revert the biblio load performed
5. Perform another bulkmarcimport with the same biblios and commandline options
6. Note the time taken to complete
7. Compare times.  The time from step 6 should be about 33% less than the time from step 2
8. Check Cataloguing and Authorities Logs to verify imported records were not logged
9. Profit!
Comment 2 Michal Denar 2020-01-09 22:07:59 UTC
Created attachment 97143 [details] [review]
Bug 23377: use ENV syspref override instead of complete disable

In an attempt to save time, bulkmarcimport temporarily sets CataloguingLog
and AuthoritiesLog to 0.  It does this by disabling syspref caching and saving
the changes to the database (then replacing the original values at completion).

Unfortunately, this disables other key sysprefs from being cached, and results in
a 50% increase in processing time for the script.

This patch instead utilizes the ENV variable override feature of sysprefs, which
preempts the cache in C4::Context->preference().

To test:
1. Perform a bulkmarcimport with a reasonable number of biblios (~1000 will do)
2. Note the time taken to complete
3. Apply patch
4. Revert the biblio load performed
5. Perform another bulkmarcimport with the same biblios and commandline options
6. Note the time taken to complete
7. Compare times.  The time from step 6 should be about 33% less than the time from step 2
8. Check Cataloguing and Authorities Logs to verify imported records were not logged
9. Profit!

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 3 Marcel de Rooy 2020-01-10 10:30:15 UTC
Looking here
Comment 4 Marcel de Rooy 2020-01-10 10:53:39 UTC
Created attachment 97183 [details] [review]
Bug 23377: use ENV syspref override instead of complete disable

In an attempt to save time, bulkmarcimport temporarily sets CataloguingLog
and AuthoritiesLog to 0.  It does this by disabling syspref caching and saving
the changes to the database (then replacing the original values at completion).

Unfortunately, this disables other key sysprefs from being cached, and results in
a 50% increase in processing time for the script.

This patch instead utilizes the ENV variable override feature of sysprefs, which
preempts the cache in C4::Context->preference().

To test:
1. Perform a bulkmarcimport with a reasonable number of biblios (~1000 will do)
2. Note the time taken to complete
3. Apply patch
4. Revert the biblio load performed
5. Perform another bulkmarcimport with the same biblios and commandline options
6. Note the time taken to complete
7. Compare times.  The time from step 6 should be about 33% less than the time from step 2
8. Check Cataloguing and Authorities Logs to verify imported records were not logged
9. Profit!

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2020-01-10 10:53:44 UTC
Created attachment 97184 [details] [review]
Bug 23377: (QA follow-up) Use OVERRIDE_SYSPREF

SYSPREF_OVERRIDE unfortunately is not supported ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2020-01-10 10:54:30 UTC
Good to see you back, Ian. Ready to QA again :)
Comment 7 Martin Renvoize (ashimema) 2020-01-10 10:57:29 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 8 Joy Nelson 2020-01-30 23:55:24 UTC
Pushed to 19.11.x branch for 19.11.03
Comment 9 Lucas Gass (lukeg) 2020-02-06 14:48:10 UTC
backported to 19.05.x for 19.05.08
Comment 10 Hayley Pelham 2020-02-10 03:00:36 UTC
Enhancement will not be backported to 18.11.x series.