more efficient way to do permit tests during opportunistic hold capture

[Galen Charlton]

Currently, one part of opportunistic hold capture is retrieving a set of up to 100 nearest candidate holds from open-ils.storage.action.hold_request.nearest_hold, then going through the list and looking for the first one that passes the permit test.

However, two problems recently seen with this are:

- For a popular title in a consortium where, generally speaking, not all copies can fill all holds on the title, it is possible to get a set of 100 candidate holds where _none_ of them pass the permit check
- Running the additional individual permit checks for each hold can take ~10 seconds in that worst case

I've experimented with putting the invocation of action.hold_retarget_permit_test() directly in the nearest_hold query and... it just might help.

[Galen Charlton (gmc)]

A WIP patch for feedback is available in the user/gmcharlt/lp2004521_add_permit_test_to_nearest_hold_query-wip working branch. Note that this branch is on top of the WIP branch for bug 2004520.

[Galen Charlton (gmc)]

Regarding the WIP patch:

[1] It will get some production testing today
[2] I did some experimentation on ways to reduce the number of times that the retarget permit test stored procedure is run.

- Wrapping the original base query in a CTE that gets joined to an outer query that actually runs the permit check didn't do it. At the moment, PostgreSQL returns the rows from an CTE in the order set in the CTE, but you can't count on that order being preserved. Using ROW_NUMBER() / OVER() in the CTE and an ORDER BY in the order query means that each row returned by the CTE gets the permit check run, since the LIMIT in the outer query gets applied *after* the ORDER BY, which in turn is *after* each expression in the WHERE clause is processed (including ones that are based on relatively expensive function calls).

- What is more promising is putting the query and the permissions checks in a stored function that runs the query, then loops over the results until it funds a hold that is permitted. It would still be a good idea to set some kind of limit on the number of holds it looks at, but that likely can be much higher than 100 because the permit checks are running directly in the database without the overhead of up to a hundred OpenSRF calls to individually invoke the retarget permit test for each candidate hold.

[3] Another thing that could help is either changing how action.hold_copy_map gets populated so that it excludes holds that the item is not actually permitted to fill (at the time of targeting), or at least to record the permit check status so that the nearest_hold logic can filter out holds that are unlikely to be available at the point of retargeting. In the later case, we'll probably want to add some logic to update the hold copy map whenever a patron undergoes a change that unbars them or removes the last penalty that blocks hold capture.

[Galen Charlton (gmc)]

Following discussion with Mike Rylander, the notion of doing permission testing before inserting rows into action.hold_copy_map of rows may not be practical: previous experiments years ago to retarget holds on the fly as patron and item conditions change caused significant database load, particular as new items made their way through the library workflow.

[Jason Stephenson (jstephenson)]

Maybe more of the work could be done in the Perl? Just throwing that suggestion out there to see what everyone thinks.

[Galen Charlton (gmc)]

Moving the logic to Perl wouldn't be quick to do, as the core hold permit checks are in-DB and would require a fair amount of work to translate back to Perl.

I'm working on a variation of the WIP that should improve on it (although the current WIP should significantly help).