Bug 36261

Summary: Calculating OpacHiddenItems is too slow in OPAC search
Product: Koha Reporter: David Cook <dcook>
Component: SearchingAssignee: David Cook <dcook>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: andreas.jonsson, hagud
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36239
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:

Description David Cook 2024-03-07 00:01:48 UTC
Calculating items and records hidden by OpacHiddenItems it slow. Too slow.

Fortunately, I've come up with an optimization which seems to effectively eliminate the overhead added by OpacHiddenItems.

Basically, instead of running 1 query per item, I run 1 query per bib to get all hidden items. It's pretty straightforward, but I'd like to battle-test it a bit more. I'm also writing this optimization for an older version of Koha, and I still have more optimizing I want to do, so I'm not going to post my patch just yet...

It's coming though!
Comment 1 Andreas Jonsson 2024-04-17 08:31:21 UTC
Ideally we should resolve this at indexing time so we get the information directly from the search index.

Also the systempreference OpacHiddenItemsHidesRecord could be implemented properly, as it now breaks the result count, and theoretically you could get a blank search result page with a positive result count, and visible results at page two.
Comment 2 David Cook 2024-04-22 00:29:15 UTC
(In reply to Andreas Jonsson from comment #1)
> Ideally we should resolve this at indexing time so we get the information
> directly from the search index.

You can't do it at indexing time, since OpacHiddenItems can be changed at any time. (The OpacHiddenItems feature in general is a bad idea imho.)

> Also the systempreference OpacHiddenItemsHidesRecord could be implemented
> properly, as it now breaks the result count, and theoretically you could get
> a blank search result page with a positive result count, and visible results
> at page two.

I don't think this is possible since the calculation is too complex. Personally, I'd rather deprecate OpacHiddenItemsHidesRecord and just require OpacSuppression to be used for any record that must be completely hidden.
Comment 3 Katrin Fischer 2024-04-22 06:16:14 UTC
We use this feature a lot of the libraries and it always depends on the use case. If you just want to hide something like on order items it works certainly well enough. But maybe it should come with a warning. I'd be against deprecating it.
Comment 4 Andreas Jonsson 2024-04-22 06:37:12 UTC
This is a setting that is rarely changed, I think it is justified to require a reindexing for changes to take effect.
Comment 5 Hugo Agud 2024-04-22 06:40:03 UTC
for us it is also widely used... I should not remove this feature.... on the other hand improve speed or reindex is not a big issue for us.. I do not know big catalogues it takes a lot.
Comment 6 David Cook 2024-04-22 06:51:50 UTC
(In reply to Andreas Jonsson from comment #4)
> This is a setting that is rarely changed, I think it is justified to require
> a reindexing for changes to take effect.

That's a good point. I suppose we do have a number of other settings that require reindexes/restarts. We could add that to the explanation.

That said, I'm still not sure it's possible at indexing time, especially since the search is used by both the OPAC and the staff interface. You couldn't just suppress entire records or items at indexing time without causing systemic problems.
Comment 7 David Cook 2024-04-22 06:55:44 UTC
(In reply to Katrin Fischer from comment #3)
> We use this feature a lot of the libraries and it always depends on the use
> case. If you just want to hide something like on order items it works
> certainly well enough. But maybe it should come with a warning. I'd be
> against deprecating it.

(In reply to Hugo Agud from comment #5)
> for us it is also widely used... I should not remove this feature.... on the
> other hand improve speed or reindex is not a big issue for us.. I do not
> know big catalogues it takes a lot.

It's one of those features that "kinda sorta works - good enough". Widely used but also widely problematic...

By the way, I wasn't talking about deprecating OpacHiddenItems but rather OpacHiddenItemsHidesRecord.

Although in theory if you did calculate OpacHiddenItems at indexing time you could turn on OpacSuppression if OpacHiddenItemsHidesRecord is enabled. That would fix some sins...
Comment 8 David Cook 2024-04-22 06:57:48 UTC
For what it's worth, I've got my performance optimization running in 23.11 production.

It doesn't solve any of the problems of OpacHiddenItemsHidesRecord or facets for hidden items appearing, but it makes a bad situation less bad in terms of performance overheads...
Comment 9 David Cook 2024-10-15 23:17:47 UTC
Apologies that I haven't had time to upstream this. Hopefully I'll have time at some point...
Comment 10 Andreas Jonsson 2024-10-17 10:38:10 UTC
(In reply to David Cook from comment #6)
> (In reply to Andreas Jonsson from comment #4)
> > This is a setting that is rarely changed, I think it is justified to require
> > a reindexing for changes to take effect.
> 
> That's a good point. I suppose we do have a number of other settings that
> require reindexes/restarts. We could add that to the explanation.
> 
> That said, I'm still not sure it's possible at indexing time, especially
> since the search is used by both the OPAC and the staff interface. You
> couldn't just suppress entire records or items at indexing time without
> causing systemic problems.

When it comes to filtering items, I'm thinking that, in the search index, there should be a record level field for "no opac visible items" and for each item there should be a flag "hidden in opac".

So, you can filter the search on the record level so you only get records that have at least one opac visible item. And then the information about which items that should be excluded in the result view is available directly from the search index.  No additional processing would be needed at search time.
Comment 11 David Cook 2024-10-18 00:18:25 UTC
(In reply to Andreas Jonsson from comment #10)
> When it comes to filtering items, I'm thinking that, in the search index,
> there should be a record level field for "no opac visible items" and for
> each item there should be a flag "hidden in opac".
> 
> So, you can filter the search on the record level so you only get records
> that have at least one opac visible item. And then the information about
> which items that should be excluded in the result view is available directly
> from the search index.  No additional processing would be needed at search
> time.

Yeah, it's an idea, and increasingly I'm thinking it could be the way to go.

We'd just need to add some explanation in the syspref description for OpacHiddenItems that they'll need to rebuild the hidden statuses if/when they make changes. (We do that with a number of other settings in Koha, so there's already a precedent, so why not, right?)

I think we'd need to build a new script for that, or maybe touch_all_items.pl would be enough. I guess we'd find out during testing...

--

So yeah... I reckon someone should give it a go. I probably won't do the coding for it, but I'd be happy to test/QA it.
Comment 12 David Cook 2024-10-18 05:55:12 UTC
(In reply to Andreas Jonsson from comment #10)
> When it comes to filtering items, I'm thinking that, in the search index,
> there should be a record level field for "no opac visible items" and for
> each item there should be a flag "hidden in opac".
> 
> So, you can filter the search on the record level so you only get records
> that have at least one opac visible item. And then the information about
> which items that should be excluded in the result view is available directly
> from the search index.  No additional processing would be needed at search
> time.

Actually, that might still not be true. If a bib record isn't hidden, then bug 14007 would probably still be a problem. 

(Although only while we're using Zebra. In the future with Elasticsearch, we might be able to tweak the document so that item data isn't returned under some conditions...)