Bug 19502

Summary: Result sets limited to 10000
Product: Koha Reporter: Nick Clemens <nick>
Component: Searching - ElasticsearchAssignee: Nick Clemens <nick>
Status: Pushed to Stable --- QA Contact: Alex Arnaud <alex.arnaud>
Severity: normal    
Priority: P5 - low CC: alex.arnaud, claire.hernandez, gaetan.boisson, jonathan.druart, julian.maurice, katrin.fischer, martin.renvoize, nick, nicolas.legrand, severine.queune
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21405
Whiteboard:
Change sponsored?: --- Patch complexity: ---
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 19502: Limit pagination to first 10000 results when using ES
Bug 19502: Limit pagination to first 10000 results when using ES
Bug 19520: (follow-up) Pass parameters to avoid making templates depend on search engine
Bug 19502: Retrieve index.max_result_window from ES
Bug 19502: Remove debug warn
Bug 19502: Add POD for max_result_window
Bug 19502: Limit pagination to first 10000 results when using ES
Bug 19502: (follow-up) Pass parameters to avoid making templates depend on search engine
Bug 19502: Retrieve index.max_result_window from ES
Bug 19502: Remove debug warn
Bug 19502: Add POD for max_result_window
Bug 19502: Limit pagination to first 10000 results when using ES
Bug 19502: (follow-up) Pass parameters to avoid making templates depend on search engine
Bug 19502: Retrieve index.max_result_window from ES
Bug 19502: Remove debug warn
Bug 19502: Add POD for max_result_window

Description Nick Clemens 2017-10-20 15:37:40 UTC
With the new feature to jump to a last page of search results it is apparent that we can't do this with our current use of ES. The standard limit is 10000 results, it can be increased at the cost of memory - need to look into 'Scroll' or 'Search after' options

https://www.elastic.co/guide/en/elasticsearch/reference/5.5/index-modules.html
 index.max_result_window
    The maximum value of from + size for searches to this index. Defaults to 10000. Search requests take heap memory and time proportional to from + size and this limits that memory. See Scroll or Search After for a more efficient alternative to raising this. 

https://www.elastic.co/guide/en/elasticsearch/reference/5.6/search-request-scroll.html

https://www.elastic.co/guide/en/elasticsearch/reference/5.6/search-request-search-after.html
Comment 1 Nick Clemens 2018-03-16 11:14:58 UTC
Created attachment 73020 [details] [review]
Bug 19502: Limit pagination to first 10000 results when using ES

This patch is to avoid hitting an error page. We should eventually make the
max number returned configurable for ES.

To test:
1 - Have Koha running ES with 10,000+ records
2 - Search for '*'
3 - Click 'Last' to view last page of results
4 - 'Cannot perform search' error
5 - Apply patch
6 - Search again
7 - View 'Last' page
8 - No error, you go to the last of 10000
9 - Note the warning above the pagination buttons
Comment 2 Séverine Queune 2018-03-16 11:29:31 UTC
Created attachment 73024 [details] [review]
Bug 19502: Limit pagination to first 10000 results when using ES

This patch is to avoid hitting an error page. We should eventually make the
max number returned configurable for ES.

To test:
1 - Have Koha running ES with 10,000+ records
2 - Search for '*'
3 - Click 'Last' to view last page of results
4 - 'Cannot perform search' error
5 - Apply patch
6 - Search again
7 - View 'Last' page
8 - No error, you go to the last of 10000
9 - Note the warning above the pagination buttons

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 3 Séverine Queune 2018-03-16 11:30:23 UTC
Works as described for both staff client and opac.
Thanks Nick !
Comment 4 Nick Clemens 2018-04-19 23:15:08 UTC
Created attachment 74587 [details] [review]
Bug 19520: (follow-up) Pass parameters to avoid making templates depend on search engine
Comment 5 Julian Maurice 2018-06-05 13:10:37 UTC
I think we can do better than hardcoding the ES default value for max_result_window. We can retrieve the actual setting from ES, so that anyone can adjust this setting depending on their needs. I'm currently preparing a patch that will do that.
Comment 6 Julian Maurice 2018-06-05 13:16:45 UTC
Created attachment 75816 [details] [review]
Bug 19502: Retrieve index.max_result_window from ES

This avoid hardcoding '10000' in two different places and allow users to
adjust this setting.

Also, this patch fixes a bug when the search return less than 10000
results

Test plan:
1. Do a search that returns 10000+ records.
2. Note the warning above the pagination buttons
3. Go to the last page, no error
4. Change the ES setting:
   curl -XPUT http://elasticsearch/koha_master_biblios/_settings -d \
     '{"index": {"max_result_window": 20000}}'
5. Do another search that returns more than 10000 but less than 20000
6. Note that the warning does not show up
7. Go to the last page, still no error
Comment 7 Julian Maurice 2018-06-05 13:18:12 UTC
Nick, can you take a look at the last patch please ?
Comment 8 Alex Arnaud 2018-06-19 13:20:28 UTC
Comment on attachment 74587 [details] [review]
Bug 19520: (follow-up) Pass parameters to avoid making templates depend on search engine

Review of attachment 74587 [details] [review]:
-----------------------------------------------------------------

::: catalogue/search.pl
@@ +677,4 @@
>                                  last_page_offset => $last_page_offset,
>                                  previous_page_offset => $previous_page_offset) unless $pages < 2;
>              $template->param(   next_page_offset => $next_page_offset) unless $pages eq $current_page_number;
> +            warn "topage $hits_to_paginate";

Debug thing ?
Comment 9 Julian Maurice 2018-06-25 12:07:30 UTC
(In reply to Alex Arnaud from comment #8)
> Debug thing ?

Yes. It can be removed.
Comment 10 Julian Maurice 2018-06-25 12:15:23 UTC
Created attachment 76371 [details] [review]
Bug 19502: Remove debug warn
Comment 11 Julian Maurice 2018-06-25 12:15:27 UTC
Created attachment 76372 [details] [review]
Bug 19502: Add POD for max_result_window
Comment 12 Katrin Fischer 2018-07-08 11:13:16 UTC
I don't have enough data to test this properly, but seems straight forward. Alex,  maybe you could have another look?
Comment 13 Alex Arnaud 2018-07-09 08:06:40 UTC
(In reply to Katrin Fischer from comment #12)
> I don't have enough data to test this properly, but seems straight forward.
> Alex,  maybe you could have another look?

I already had many looks. I'd like Nick to valid/sign Julian's patches and i could pass QA.
Comment 14 Katrin Fischer 2018-07-09 09:50:29 UTC
(In reply to Alex Arnaud from comment #13)
> (In reply to Katrin Fischer from comment #12)
> > I don't have enough data to test this properly, but seems straight forward.
> > Alex,  maybe you could have another look?
> 
> I already had many looks. I'd like Nick to valid/sign Julian's patches and i
> could pass QA.

Sounds like a plan.
Comment 15 Julian Maurice 2018-07-09 10:08:03 UTC
I wished Nick would review my patch to be sure that it doesn't go against his first patches. But I think this should not prevent the QA to be done. He can review the patches while pushing them ;)
Comment 16 Julian Maurice 2018-07-09 10:11:17 UTC
(In reply to Katrin Fischer from comment #12)
> I don't have enough data to test this properly, but seems straight forward.
> Alex,  maybe you could have another look?

If you want, here is a tiny script to generate thousands of new biblio: https://nopaste.xyz/?6a713982d73e1ab2#44I4RKe2ynghhda8Gp48a9f3bQSXjWbPUoeawx1Npqc= (You may want to change the title field for MARC21)
Comment 17 Nick Clemens 2018-07-09 11:51:46 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2018-07-09 11:51:49 UTC Comment hidden (obsolete)
Comment 19 Nick Clemens 2018-07-09 11:51:53 UTC Comment hidden (obsolete)
Comment 20 Nick Clemens 2018-07-09 11:51:56 UTC Comment hidden (obsolete)
Comment 21 Nick Clemens 2018-07-09 11:52:00 UTC Comment hidden (obsolete)
Comment 22 Alex Arnaud 2018-07-09 13:31:55 UTC
Created attachment 76794 [details] [review]
Bug 19502: Limit pagination to first 10000 results when using ES

This patch is to avoid hitting an error page. We should eventually make the
max number returned configurable for ES.

To test:
1 - Have Koha running ES with 10,000+ records
2 - Search for '*'
3 - Click 'Last' to view last page of results
4 - 'Cannot perform search' error
5 - Apply patch
6 - Search again
7 - View 'Last' page
8 - No error, you go to the last of 10000
9 - Note the warning above the pagination buttons

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 23 Alex Arnaud 2018-07-09 13:32:04 UTC
Created attachment 76795 [details] [review]
Bug 19502: (follow-up) Pass parameters to avoid making templates depend on search engine

https://bugs.koha-community.org/show_bug.cgi?id=19502

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 24 Alex Arnaud 2018-07-09 13:32:16 UTC
Created attachment 76796 [details] [review]
Bug 19502: Retrieve index.max_result_window from ES

This avoid hardcoding '10000' in two different places and allow users to
adjust this setting.

Also, this patch fixes a bug when the search return less than 10000
results

Test plan:
1. Do a search that returns 10000+ records.
2. Note the warning above the pagination buttons
3. Go to the last page, no error
4. Change the ES setting:
   curl -XPUT http://elasticsearch/koha_master_biblios/_settings -d \
     '{"index": {"max_result_window": 20000}}'
5. Do another search that returns more than 10000 but less than 20000
6. Note that the warning does not show up
7. Go to the last page, still no error

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 25 Alex Arnaud 2018-07-09 13:32:27 UTC
Created attachment 76798 [details] [review]
Bug 19502: Remove debug warn

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 26 Alex Arnaud 2018-07-09 13:32:38 UTC
Created attachment 76799 [details] [review]
Bug 19502: Add POD for max_result_window

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Alex Arnaud <alex.arnaud@biblibre.com>
Comment 27 Katrin Fischer 2018-07-09 18:33:52 UTC
yay!
Comment 28 Nick Clemens 2018-07-13 11:24:37 UTC
Awesome work all!

Pushed to master for 18.11
Comment 29 Martin Renvoize 2018-07-15 18:51:24 UTC
Pushed to 18.05.x for 18.05.02