Speed up hold copy map deletion for clear shelf process, etc.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1386347-delete-hold-copy-maps-proc

Note I did not modify the main hold targeter, since it's built from open-ils.storage talking directly to the DB. It should not suffer from the same overhead as the middle layer calls.

I wonder if, instead of depending on the app to manage this at fulfillment or cancel time, we shouldn't simply attach an ON UPDATE trigger to action.hold_request that clears the hold_copy_map at the appropriate time, and remove the app-level code that wants to be responsible for that. This would not, of course, change the targetter (much as your proposal does not) as that is an interim process that will end up refilling the map. The only prerequisite is that there needs to be a set of conditions on the hold (cancel time not null, or the like) that the trigger can be attached to, or test.

Thoughts?

Thanks, Mike. Even better...

These conditions come to mind:

1. cancel_time IS NOT NULL
 -- of course, consistent w/ current behavior.

2. fulfillment_time IS NOT NULL
 -- consistent w/ current behavior.
 -- Hmm, maybe we could check capture_time instead? We don't do opportunistic capture on captured holds and the copy maps will be regenerated anyway (IIRC) if it's manually retargeted.

3. frozen is true
-- I don't think we clear the copy maps now when a hold goes from unfrozen to frozen, but it seems like we might as well. A hold that starts out frozen is never targeted in the first place.

Thanks, Bill.

I think I'd leave condition (3) as-is, as it saves the system a little work when a hold is frozen and then thawed in short order, such as when the patron freezes a hold by mistake.

We can't fulfill without capture, so the change in (2) makes sense.

Here's a branch: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/clear_hold_copy_map

Doh! I should really announce my intentions to work on code ;) I just pushed user/berick/lp1386347-delete-hold-copy-maps-tgr, which does the same thing, minus condition 3, which makes sense. I'll give yours a test Mike. Thanks again.

Added a small fix (the trigger wanted a RETURN), squashed, tested and signed of at:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/clear_hold_copy_map

Confirmed that capture and cancel clear the copy maps and freezing does not.

Yay for making things more efficient! Since this is a fairly important change, I'll leave this unmerged for a bit in case anyone else would like to test.

/me uncrosses the streams. The branches were indeed very similar!

For posterity, using the WHEN clause on the trigger will eliminate extra invocations of the trigger function when it's not needed.

Thanks, Bill.

Cool. And I just found two additional cases of no longer needed map deletions Fix pushed to same branch: user/berick/clear_hold_copy_map.

One issue I've discovered with the change in when copy maps are deleted (captured vs. fulfilled), is that the queue position and # potential copies reported on capture holds now shows holds as being at the back of the line with zero potential copies. (I'll spare the details, but the crux is that w/o potential copies, the queue data gets thrown off).

Do we return to deleting copy maps when the hold is fulfilled instead of captured OR do we skip the queue calculation for captured holds and simply return position=0, potentials=1. The former is simple and returns the status quo, the latter produces different data, but allows for some efficiency (skipping queue calc).

Returning hard-coded queue values for capture holds is more honest in the sense that a captured hold really is at the front of the line, however we no longer have access to its queue position and # copies at the time of capture, without resetting (un-capturing) the hold, which is information staff may have relied on.

My preference would be to maintain the API for the time being. While position calculation is expensive, it's less so these days than in the past. I think there's a good documented optimization opportunity here, if we want to do that as a follow-on patch.

Thanks for the input, Mike. I've pushed a new branch, rebased to master, which deletes copy maps at fulfillment time instead of capture time, as before, to retain existing API.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1386347-clear-hold-copy-map

You'll find a signoff branch at: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1386347-clear-hold-copy-map-signoff

Thanks for the sign-offs, Mike. I've merged the code to master (2.8-beta). I did not merge to 2.7/2.6, pending thumbs up or down from RM's. Since this one is in a gray area and there's no outcry to have it back-ported, I'm personally leaning toward not back-porting.

Removing older milestones, since the general decision was made not to backport this to previous versions but only apply it towards master and 2.8+.