Bug 35579 - marcrecord2csv searches authorised values inefficiently
Summary: marcrecord2csv searches authorised values inefficiently
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Julian Maurice
URL:
Keywords:
Depends on:
Blocks: 35588
  Show dependency treegraph
 
Reported: 2023-12-15 05:12 UTC by David Cook
Modified: 2024-01-12 22:03 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This significantly improves the speed of downloading large lists in CSV format. (It adds a get_descriptions_by_marc_field" method which caches AuthorisedValue descriptions when searched by MARC field, which is used when exporting MARC to CSV.)
Version(s) released in:
24.05.00,23.11.02,23.05.08


Attachments
Bug 35579: Cache authorised value lookup by MARC field (3.65 KB, patch)
2023-12-15 06:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 35579: Add unit test (1.46 KB, patch)
2023-12-18 00:12 UTC, David Cook
Details | Diff | Splinter Review
Bug 35579: Cache authorised value lookup by MARC field (3.70 KB, patch)
2023-12-18 18:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 35579: Add unit test (1.51 KB, patch)
2023-12-18 18:40 UTC, David Nind
Details | Diff | Splinter Review
Bug 35579: Cache authorised value lookup by MARC field (3.76 KB, patch)
2023-12-22 12:48 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 35579: Add unit test (1.57 KB, patch)
2023-12-22 12:48 UTC, Julian Maurice
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-12-15 05:12:06 UTC
If you have a CSV export profile with a large number of fields, and you have a list with a large number of records, you end up making the exact same database calls thousands of times.

This chews up CPU like crazy and it can also mean your list never downloads as the proxy times out. 

So we need to cache! 

Koha::AuthorisedValues already caches when getting descriptions by "koha_field", but it does *not* when getting descriptions by "marc_field".

Koha::AuthorisedValues->search_by_marc_field is actually only ever used by C4::Record for CSV exports anyways so that's probably why this hasn't come up before...
Comment 1 David Cook 2023-12-15 05:13:02 UTC
In koha-testing-docker, after making a list, you can use SQL like the following to populate the list with all the bib records in Koha:

insert into virtualshelfcontents (shelfnumber,biblionumber,borrowernumber) select 1,biblionumber,51 from biblio;
Comment 2 David Cook 2023-12-15 05:13:32 UTC
And when making a CSV profile, you'll want "Profile MARC fields" like so:

000|001|003|005|006|007|008|010|015|016|020|022|040|050|082|100|110|111|130|240|243|245|246|247|250|260|300|336|337|338|440|490|500|501|505|520|530|600|610|611|630|648|650|651|690|700|710|711|856|887|942|995
Comment 3 David Cook 2023-12-15 06:04:45 UTC
Created attachment 159858 [details] [review]
Bug 35579: Cache authorised value lookup by MARC field

This patch adds a "get_descriptions_by_marc_field" method
which caches AuthorisedValue descriptions when searched by
MARC field, which is used when exporting MARC to CSV.
Comment 4 David Cook 2023-12-15 06:05:42 UTC
I've run out of time for tests, but I figured I'd post the patch anyway. I can come back for tests next week...

On koha-testing-docker, this changes a MARC to CSV output from 1.3 minutes to about 7 seconds when exporting the full MARC database.

So that's nice :)
Comment 5 David Cook 2023-12-18 00:02:29 UTC
While fixing the issue, I discovered bug 35588...
Comment 6 David Cook 2023-12-18 00:12:01 UTC
Created attachment 159936 [details] [review]
Bug 35579: Add unit test
Comment 7 David Cook 2023-12-18 00:16:31 UTC
Test plan:
0. Do not apply the patch yet!
1. Go to http://localhost:8081/cgi-bin/koha/tools/csv-profiles.pl
2. Add a CSV profile with the default values and using the following
for the "Profile MARC fields":
000|001|003|005|006|007|008|010|015|016|020|022|040|050|082|100|110|111|130|240|243|245|246|247|250|260|300|336|337|338|440|490|500|501|505|520|530|600|610|611|630|648|650|651|690|700|710|711|856|887|942|995

3. Create a new List
4. Add all the database's bibs to that list using SQL like the following (where the shelfnumber equals the number for your list):
insert into virtualshelfcontents (shelfnumber,biblionumber,borrowernumber) select 1,biblionumber,51 from biblio;

5. Go to that list in the staff interface
6. Download that list using your CSV profile

7. Apply the patch
8. koha-plack --reload kohadev

9. Download that list using your CSV profile
10. Note that the output is the same, but that the output completes much more quickly after applying the patch
Comment 8 David Nind 2023-12-18 18:40:54 UTC
Created attachment 159975 [details] [review]
Bug 35579: Cache authorised value lookup by MARC field

This patch adds a "get_descriptions_by_marc_field" method
which caches AuthorisedValue descriptions when searched by
MARC field, which is used when exporting MARC to CSV.

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2023-12-18 18:40:56 UTC
Created attachment 159976 [details] [review]
Bug 35579: Add unit test

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2023-12-18 18:49:11 UTC
Testing notes (using KTD):

1. Download speeds (using Web Developer Tools > Network feature in Firefox):
   . Before: 1 request  270.81 kB / 336 B transferred  Finish: 46.66 s
   . Refreshed browser, flush_memcached, restart_all (so everything fresh)
   . After : 1 request  270.81 kB / 336 B transferred  Finish: 4.03 s 

2. Compared downloaded files using a diff tool - no differences identified.
Comment 11 David Cook 2023-12-19 03:05:37 UTC
(In reply to David Nind from comment #10)
> Testing notes (using KTD):
> 
> 1. Download speeds (using Web Developer Tools > Network feature in Firefox):
>    . Before: 1 request  270.81 kB / 336 B transferred  Finish: 46.66 s
>    . Refreshed browser, flush_memcached, restart_all (so everything fresh)
>    . After : 1 request  270.81 kB / 336 B transferred  Finish: 4.03 s 
> 
> 2. Compared downloaded files using a diff tool - no differences identified.

You're a champion, David!
Comment 12 Julian Maurice 2023-12-22 12:48:27 UTC
Created attachment 160234 [details] [review]
Bug 35579: Cache authorised value lookup by MARC field

This patch adds a "get_descriptions_by_marc_field" method
which caches AuthorisedValue descriptions when searched by
MARC field, which is used when exporting MARC to CSV.

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 13 Julian Maurice 2023-12-22 12:48:29 UTC
Created attachment 160235 [details] [review]
Bug 35579: Add unit test

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 14 Julian Maurice 2023-12-22 12:52:05 UTC
Works as expected, no complaints from QA script and tests pass.
Passed QA
Comment 15 Katrin Fischer 2023-12-28 09:59:14 UTC
It looks like tagsubfield is optional in get_descriptions_by_marc_field, but there is no unit test for this case?

The example given in the test plan doesn't seem to include any fields that are linked to an AV category.

000|001|003|005|006|007|008|010|015|016|020|022|040|050|082|100|110|111|130|240|243|245|246|247|250|260|300|336|337|338|440|490|500|501|505|520|530|600|610|611|630|648|650|651|690|700|710|711|856|887|942|995

Or maybe only UNIMARC (995). I amended the test to also include 952 and tried single subfields like 952$c, 952$8 as well.
Comment 16 Katrin Fischer 2023-12-28 10:00:24 UTC
... and please also include test plans in commit messages :)
Comment 17 Katrin Fischer 2023-12-28 11:17:11 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Fridolin Somers 2024-01-03 16:04:33 UTC
Pushed to 23.11.x for 23.11.02
Comment 19 Lucas Gass 2024-01-12 22:03:35 UTC
Backported to 23.05.x for upcoming 23.05.08