[Bug 35579] New: marcrecord2csv searches authorised values inefficiently
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Bug ID: 35579 Summary: marcrecord2csv searches authorised values inefficiently Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: normal Priority: P5 - low Component: MARC Bibliographic data support Assignee: koha-bugs@lists.koha-community.org Reporter: dcook@prosentient.com.au QA Contact: testopia@bugs.koha-community.org 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... -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #1 from David Cook <dcook@prosentient.com.au> --- 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; -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- Created attachment 159858 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159858&action=edit 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- 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 :) -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |dcook@prosentient.com.au |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=34051 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|MARC Bibliographic data |Tools |support | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=35588 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #5 from David Cook <dcook@prosentient.com.au> --- While fixing the issue, I discovered bug 35588... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |35588 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35588 [Bug 35588] marcrecord2csv retrieves authorised values incorrectly for fields -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #6 from David Cook <dcook@prosentient.com.au> --- Created attachment 159936 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159936&action=edit Bug 35579: Add unit test -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #7 from David Cook <dcook@prosentient.com.au> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159858|0 |1 is obsolete| | --- Comment #8 from David Nind <david@davidnind.com> --- Created attachment 159975 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159975&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159936|0 |1 is obsolete| | --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 159976 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=159976&action=edit Bug 35579: Add unit test Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com Text to go in the| |This significantly improves release notes| |the speed of downloading | |large lists in CSV format. | |(It adds a | |get_descriptions_by_marc_fi | |eld" method which caches | |AuthorisedValue | |descriptions when searched | |by MARC field, which is | |used when exporting MARC to | |CSV.) --- Comment #10 from 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #11 from David Cook <dcook@prosentient.com.au> --- (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! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com QA Contact|testopia@bugs.koha-communit |julian.maurice@biblibre.com |y.org | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159975|0 |1 is obsolete| | --- Comment #12 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 160234 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160234&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #159976|0 |1 is obsolete| | --- Comment #13 from Julian Maurice <julian.maurice@biblibre.com> --- Created attachment 160235 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=160235&action=edit Bug 35579: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #14 from Julian Maurice <julian.maurice@biblibre.com> --- Works as expected, no complaints from QA script and tests pass. Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #15 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- ... and please also include test plans in commit messages :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Pushed for 24.05! Well done everyone, thank you! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolin.somers@biblibre.co | |m Status|Pushed to master |Pushed to stable Version(s)|24.05.00 |24.05.00,23.11.02 released in| | --- Comment #18 from Fridolin Somers <fridolin.somers@biblibre.com> --- Pushed to 23.11.x for 23.11.02 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00,23.11.02 |24.05.00,23.11.02,23.05.08 released in| | Status|Pushed to stable |Pushed to oldstable CC| |lucas@bywatersolutions.com --- Comment #19 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 23.05.x for upcoming 23.05.08 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35579 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=40554 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org