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...
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;
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
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.
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 :)
While fixing the issue, I discovered bug 35588...
Created attachment 159936 [details] [review] Bug 35579: Add unit test
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
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>
Created attachment 159976 [details] [review] Bug 35579: Add unit test Signed-off-by: David Nind <david@davidnind.com>
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.
(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!
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>
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>
Works as expected, no complaints from QA script and tests pass. Passed QA
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.
... and please also include test plans in commit messages :)
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.02
Backported to 23.05.x for upcoming 23.05.08