https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kyle@bywatersolutions.com |ity.org | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=43101 -- 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=43102 --- Comment #1 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 202108 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202108&action=edit Bug 43102: Koha::Items filters inline every itemnumber of the set as an OR chain filter_by_checked_out, filter_by_in_transit, filter_by_has_holds and filter_by_has_recalls already restrict the final search with an IN subquery, but the subquery itself is built by fetching every itemnumber of the set into memory and passing the list as an arrayref that SQL::Abstract renders as one OR clause per itemnumber. For a records with large numbers of items this can produce very large queries. This patch passes the itemnumbers as a subquery so the database resolves the set itself. Existing unit tests should prove there is no change in behavior. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note all tests pass! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #3 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 202110 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202110&action=edit Bug 43102: Add POD to _extract_custom_attrs and _process_params Patch from commit fdbbad7 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 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=43102 --- Comment #4 from David Nind <david@davidnind.com> --- Created attachment 202112 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202112&action=edit Bug 43102: Koha::Items filters inline every itemnumber of the set as an OR chain filter_by_checked_out, filter_by_in_transit, filter_by_has_holds and filter_by_has_recalls already restrict the final search with an IN subquery, but the subquery itself is built by fetching every itemnumber of the set into memory and passing the list as an arrayref that SQL::Abstract renders as one OR clause per itemnumber. For a records with large numbers of items this can produce very large queries. This patch passes the itemnumbers as a subquery so the database resolves the set itself. Existing unit tests should prove there is no change in behavior. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note all tests pass! 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=43102 --- Comment #6 from David Nind <david@davidnind.com> --- Created attachment 202114 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=202114&action=edit Bug 43102: Add POD to _extract_custom_attrs and _process_params 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=43102 --- Comment #7 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Hi Kyle, I checked this out and was going to pass QA but Opus has found two things that I think are worth a look. I validated it and it seems these are real: 1) The itemnumber subquery carries the whole resultset, not just the id. get_column('itemnumber')->as_query inherits $self's column list and ORDER BY, so DBIC wraps a derived table rather than emitting a plain id subquery: SELECT me.itemnumber FROM (SELECT <all 48 item columns> FROM items me JOIN biblioitems ... LEFT JOIN branches ... ORDER BY ...) me filter_by_available nests eight of those - a constant 12.7 KB of SQL, even for a record with one item. Trimming the subquery to the one column it needs drops that to 3.9 KB and roughly halves the time again (~30 ms vs ~58 ms at 500 items, median of 3, REST caller shape). 2) The filters now break on a resultset carrying a LIMIT. MariaDB does not allow LIMIT inside an IN subquery: $biblio->items->search({}, { rows => 3 })->filter_by_available->as_list # DBI Exception: DBD::mysql::st execute failed: This version of MariaDB # doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery' All five filters are affected - filter_by_available, filter_by_checked_out, filter_by_in_transit, filter_by_has_holds, filter_by_has_recalls - and all five worked before the patches. No in-tree caller hits it: filters and paging go to the same search() call, so filter_by_* runs unpaged and SUPER::search puts the LIMIT outermost. So it's a public API contract change rather than a live bug, but it fails opaquely, and ->count returns undef instead of propagating the error. Both have the same fix - trim the subquery, then put the derived table back deliberately: $self->_resultset->search( undef, { columns => ['itemnumber'], order_by => undef } ) ->as_subselect_rs->get_column('itemnumber')->as_query on all five occurrences. as_subselect_rs is what keeps LIMIT legal, the column trim is what shrinks the query. Tested with that applied as real code: the same 93 tests pass, available-item sets are identical to the pre-patch behaviour across 50 randomised comparisons, and all five LIMIT cases above behave as they did before. Careful if you try this: dropping the ORDER BY without adding as_subselect_rs makes the LIMIT problem worse. The inherited ORDER BY is forcing the derived-table wrap by accident today, and it's the only reason the search_ordered shape still works - remove it and that case fails too. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #202113|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #209516|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #11 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209518 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209518&action=edit Bug 43102: filter_by_available computes the set difference in Perl filter_by_available fetches every itemnumber of the set and of each not-available subset into memory, subtracts them with array_minus, and inlines the result into the final query as one OR clause per itemnumber. This patch keeps the whole operation in the database by chaining NOT IN subqueries for the not-available sets onto the availability conditions. Existing unit tests should prove the fixes have caused no regression in existing behavior. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note all tests pass! 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=43102 --- Comment #13 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209520 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209520&action=edit Bug 43102: (QA follow-up) Add unit tests These tests demonstrate that the item filters fail on a resultset carrying a LIMIT. MariaDB doesn't support LIMIT inside an IN subquery, so all five filters die with "This version of MariaDB doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery'". They also check that a filter on an ordered resultset with a LIMIT only considers the items that resultset contains. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note the new subtest fails! 4) Apply the following patch 5) prove t/db_dependent/Koha/Items.t 6) Note all tests pass! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #15 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209522 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209522&action=edit Bug 43102: (QA follow-up) Add unit test for filter_by_available on a limited resultset Before this bug, filter_by_available returned the available items among those the resultset contains. It now builds on $self->search, so the LIMIT is applied after the availability conditions and the result can include items that were never in the resultset. For a resultset of a record's two oldest items where the oldest is checked out, filter_by_available should return just the second oldest item. Instead it returns the second and third oldest items. This patch adds a unit test that demonstrates the problem. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note the test fails! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #209518|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #209520|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #209522|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #17 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209528 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209528&action=edit Bug 43102: filter_by_available computes the set difference in Perl filter_by_available fetches every itemnumber of the set and of each not-available subset into memory, subtracts them with array_minus, and inlines the result into the final query as one OR clause per itemnumber. This patch keeps the whole operation in the database by chaining NOT IN subqueries for the not-available sets onto the availability conditions. Existing unit tests should prove the fixes have caused no regression in existing behavior. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note all tests pass! 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=43102 --- Comment #19 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209530 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209530&action=edit Bug 43102: (QA follow-up) Add unit tests The filters die on a resultset with a LIMIT, MariaDB doesn't support LIMIT inside an IN subquery. Also checks that a filter only considers the items an ordered, limited resultset contains. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #21 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- Created attachment 209532 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209532&action=edit Bug 43102: (QA follow-up) Add unit test for filter_by_available on a limited resultset For a record's two oldest items with the oldest checked out, filter_by_available returns the second and third oldest items instead of just the second. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Paul Derscheid <paul.derscheid@lmscloud.de> 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=43102 --- Comment #23 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 210217 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=210217&action=edit Bug 43102: Koha::Items filters inline every itemnumber of the set as an OR chain filter_by_checked_out, filter_by_in_transit, filter_by_has_holds and filter_by_has_recalls already restrict the final search with an IN subquery, but the subquery itself is built by fetching every itemnumber of the set into memory and passing the list as an arrayref that SQL::Abstract renders as one OR clause per itemnumber. For a records with large numbers of items this can produce very large queries. This patch passes the itemnumbers as a subquery so the database resolves the set itself. Existing unit tests should prove there is no change in behavior. Test Plan: 1) Apply this patch 2) prove t/db_dependent/Koha/Items.t 3) Note all tests pass! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #25 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 210219 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=210219&action=edit Bug 43102: Add POD to _extract_custom_attrs and _process_params Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #27 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 210221 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=210221&action=edit Bug 43102: (QA follow-up) Trim itemnumber subqueries to a single column * Moves the itemnumber subqueries into one helper that selects only the itemnumber column, they carried every item column * Wraps the subquery in a derived table with as_subselect_rs, MariaDB doesn't allow LIMIT inside an IN subquery * Keeps the inherited ORDER BY so a limited resultset still selects the same items, the database ignores it when there is no LIMIT Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #29 from Paul Derscheid <paul.derscheid@lmscloud.de> --- Created attachment 210223 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=210223&action=edit Bug 43102: (QA follow-up) Restrict filter_by_available to the items in the resultset filter_by_available applied a resultset's LIMIT after the availability conditions, so it could return items from outside the resultset. It now only considers the resultset's own items, like the other filters. Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |pedro.amorim@openfifth.co.u | |k Attachment #210443|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=43102 --- Comment #31 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Thanks everyone! Pushed to main for 26.11! -- You are receiving this mail because: You are watching all bug changes.