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

Bug #1386347 reported by Bill Erickson on 2014-10-27
This bug affects 1 person
Affects Status Importance Assigned to Milestone

Bug Description

Evergreen all versions circ 2.7

During the holds clear shelf process, holds fulfillment (checkout), and patron/staff hold cancellation, the middle layer circ code fetches all of the hold copy maps then deletes them each individually since they are no longer needed. For a hold with hundreds of copies in the map, this process can add noticeable time (1-2 seconds for me). For a single hold, this is not a big deal, but for the clear shelf process, where hundreds of holds may be canceled, deleting the hold copy maps can add a significant amount of time overall. If enough holds are processed, it increases the likelihood of a timeout in the client.

I'd like to propose a DB stored proc. which simply deletes all hold copy maps for a given hold. Something along the lines of:

action.clear_hold_copy_maps(hold_id) -> delete from action.hold_copy_map where hold = hold_id;

This would allow us to avoid all of the delete call round trips to cstore.

Holds.pm:delete_hold_copy_maps() would call this new procedure instead of fetching / deleting each individual map.

Bill Erickson (berick) wrote :


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.

tags: added: pullrequest
Changed in evergreen:
milestone: none → 2.7.1
Mike Rylander (mrylander) wrote :

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.


Bill Erickson (berick) on 2014-10-28
tags: removed: pullrequest
Bill Erickson (berick) wrote :

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.

Mike Rylander (mrylander) wrote :

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

tags: added: pullrequest
Bill Erickson (berick) wrote :

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.

Bill Erickson (berick) wrote :

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


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.

Mike Rylander (mrylander) wrote :

/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.

Bill Erickson (berick) wrote :

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.

Changed in evergreen:
milestone: 2.7.1 → 2.7.2
Bill Erickson (berick) wrote :

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.

Mike Rylander (mrylander) wrote :

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.

Bill Erickson (berick) wrote :

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.


Galen Charlton (gmc) on 2014-12-23
Changed in evergreen:
milestone: 2.7.2 → 2.7.3
Bill Erickson (berick) wrote :

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.

Ben Shum (bshum) wrote :

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+.

Changed in evergreen:
milestone: 2.7.3 → 2.8-beta
status: New → Fix Committed
no longer affects: evergreen/2.8
no longer affects: evergreen/2.6
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers