Bug 31222 - DBIC queries for batch mod can be very large
Summary: DBIC queries for batch mod can be very large
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-07-22 14:47 UTC by Nick Clemens
Modified: 2023-06-08 22:26 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00,22.05.06,21.11.12


Attachments
Bug 31222: Reduce query size for batchMod (2.94 KB, patch)
2022-07-22 14:48 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31222: Reduce query size for batchMod (3.00 KB, patch)
2022-08-05 20:43 UTC, Rachael
Details | Diff | Splinter Review
Bug 31222: Reduce query size for batchMod (3.07 KB, patch)
2022-08-17 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2022-07-22 14:47:30 UTC
We had an issue with  mysql logging for a site doing many batch modifications.

The object search is currently:
Koha::Items->search({ barcode => \@contentlist }

Which generate code like:
barcode = 1 OR barcode = 2 OR barcode = 3 ....

This can get quite large

We can reduce the query size by using -in:
Koha::Items->search({ barcode => { -in => \@contentlist } }

Which generates code like:
barcode in ( 1, 2, 3 )
Comment 1 Nick Clemens 2022-07-22 14:48:34 UTC
Created attachment 138038 [details] [review]
Bug 31222: Reduce query size for batchMod

The object search is currently:
Koha::Items->search({ barcode => \@contentlist }

Which generate code like:
barcode = 1 OR barcode = 2 OR barcode = 3 ....

This can get quite large

We can reduce the query size by using -in:
Koha::Items->search({ barcode => { -in => \@contentlist } }

Which generates code like:
barcode in ( 1, 2, 3 )

To test:
1 - Apply patch
2 - Perform batch modifications
3 - Confirm nothing has changed
Comment 2 Rachael 2022-08-02 18:21:55 UTC
Batch modifications changes each item. Ex. Modifying the Source of classification or shelving scheme to Dewey Decimal Classification gets applied.
Comment 3 Nick Clemens 2022-08-03 10:15:16 UTC
(In reply to Rachael from comment #2)
> Batch modifications changes each item. Ex. Modifying the Source of
> classification or shelving scheme to Dewey Decimal Classification gets
> applied.

Hi Rachel,  I am not sure what you mean here? Batch modification should update each item, is it updating more fields than intended?
Comment 4 Rachael 2022-08-03 12:47:08 UTC
(In reply to Nick Clemens from comment #3)
> (In reply to Rachael from comment #2)
> > Batch modifications changes each item. Ex. Modifying the Source of
> > classification or shelving scheme to Dewey Decimal Classification gets
> > applied.
> 
> Hi Rachel,  I am not sure what you mean here? Batch modification should
> update each item, is it updating more fields than intended?

Hi Nick,
No, nothing else changes, just the fields that I specified. Should I sign it off then?
Comment 5 Nick Clemens 2022-08-05 20:06:40 UTC
(In reply to Rachael from comment #4)
> (In reply to Nick Clemens from comment #3)
> > (In reply to Rachael from comment #2)
> > > Batch modifications changes each item. Ex. Modifying the Source of
> > > classification or shelving scheme to Dewey Decimal Classification gets
> > > applied.
> > 
> > Hi Rachel,  I am not sure what you mean here? Batch modification should
> > update each item, is it updating more fields than intended?
> 
> Hi Nick,
> No, nothing else changes, just the fields that I specified. Should I sign it
> off then?

If everything works as expected with the patch, yes, please :-)
Comment 6 Rachael 2022-08-05 20:43:42 UTC
Created attachment 138737 [details] [review]
Bug 31222: Reduce query size for batchMod

The object search is currently:
Koha::Items->search({ barcode => \@contentlist }

Which generate code like:
barcode = 1 OR barcode = 2 OR barcode = 3 ....

This can get quite large

We can reduce the query size by using -in:
Koha::Items->search({ barcode => { -in => \@contentlist } }

Which generates code like:
barcode in ( 1, 2, 3 )

To test:
1 - Apply patch
2 - Perform batch modifications
3 - Confirm nothing has changed

Signed-off-by: Rachael Laritz <rachael.laritz@inlibro.com>
Comment 7 Jonathan Druart 2022-08-09 08:00:12 UTC
Hum, what was the thing with the IN limit?
Comment 8 Jonathan Druart 2022-08-09 08:01:33 UTC
IN limit is max_allowed_packet, which is 16M by default.
Comment 9 Jonathan Druart 2022-08-09 08:03:42 UTC
Why do we want to reduce mysql query length exactly? You are not supposed to have mysql logging in production servers, you will face perf issues (but I guess you know that already).
Comment 10 Jonathan Druart 2022-08-17 09:50:39 UTC
Waiting for a reply.
Comment 11 Nick Clemens 2022-08-17 11:26:55 UTC
(In reply to Jonathan Druart from comment #9)
> Why do we want to reduce mysql query length exactly? You are not supposed to
> have mysql logging in production servers, you will face perf issues (but I
> guess you know that already).

We are logging, we seem to have it setup to avoid performance pitfalls, but larger queries can start to take up space.

Style wise too I think it reads better, and makes query easier to read in debug.

It seems IN is more performant as well:
https://mariadb.com/kb/en/in/
Comment 12 Jonathan Druart 2022-08-17 13:17:12 UTC
(In reply to Nick Clemens from comment #11)
> It seems IN is more performant as well:
> https://mariadb.com/kb/en/in/

That's the correct argument ;)
Comment 13 Jonathan Druart 2022-08-17 13:17:47 UTC
Created attachment 139293 [details] [review]
Bug 31222: Reduce query size for batchMod

The object search is currently:
Koha::Items->search({ barcode => \@contentlist }

Which generate code like:
barcode = 1 OR barcode = 2 OR barcode = 3 ....

This can get quite large

We can reduce the query size by using -in:
Koha::Items->search({ barcode => { -in => \@contentlist } }

Which generates code like:
barcode in ( 1, 2, 3 )

To test:
1 - Apply patch
2 - Perform batch modifications
3 - Confirm nothing has changed

Signed-off-by: Rachael Laritz <rachael.laritz@inlibro.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 14 Tomás Cohen Arazi 2022-08-17 13:25:31 UTC
(In reply to Jonathan Druart from comment #12)
> (In reply to Nick Clemens from comment #11)
> > It seems IN is more performant as well:
> > https://mariadb.com/kb/en/in/
> 
> That's the correct argument ;)

+1
Comment 15 Tomás Cohen Arazi 2022-08-17 18:52:54 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 16 David Cook 2022-08-18 02:08:56 UTC
(In reply to Jonathan Druart from comment #8)
> IN limit is max_allowed_packet, which is 16M by default.

(In reply to Jonathan Druart from comment #9)
> Why do we want to reduce mysql query length exactly? You are not supposed to
> have mysql logging in production servers, you will face perf issues (but I
> guess you know that already).

While I don't think I've seen it in Koha, I have seen other systems throw MySQL errors, because their SQL queries were too long. Of course "barcode in(?, ?, ?)" could still have that happen, but I suppose it's less likely than "barcode = ? or barcode = ? or barcode = ?". 

--

I keep looking at https://mariadb.com/kb/en/in/, but it doesn't really make sense outside their limited examples.

In the example 'SELECT 2 IN (0,3,5,7);' it makes sense that it would do a binary search of that value list, since that would be the most efficient operation. It would just need to do 1 search.

However, if it's "SELECT barcode WHERE barcode IN (1,2,3,4,5)", then it would be extremely inefficient to binary search the value list, because it would have to row scan the whole table for each "barcode" field and then binary search the value list.

At a glance, it looks like both query styles actually have the same performance:

analyze select * from items where barcode = '1' or barcode = '2' or barcode = '3' or barcode = '4' or barcode = '5';

analyze select * from items where barcode in ('1','2','3','4','5');

Both queries do a range query using the itembarcodeidx. 

Now that idea of the database row scanning the whole table and then binary searching the value list can still happen, if the number of rows in the database is < the number of values in the value list. 

For example:
analyze select * from z3950servers where id in ('1','2','3','4','5');

That will do an ALL type query (ie table scan of every row).
Comment 17 Lucas Gass 2022-10-03 22:39:12 UTC
Backported to 22.05.x for 22.05.06
Comment 18 Victor Grousset/tuxayo 2022-10-16 21:45:53 UTC
Not backported to oldoldstable (21.05.x). Feel free to ask if it's needed.

Nothing to document it seems, marking resolved.
Comment 19 Arthur Suzuki 2022-10-22 22:08:55 UTC
Thanks!

Pushed to 21.11 for 21.11.12