possible optimization for evergreen.ranked_volumes database function

[Jason Stephenson]

From depesz's email to the developer list:

my name is Hubert Lubaczewski (a.k.a. depesz). I'm PostgreSQL DBA from
Poland, that was hired to do review for Evergreen installations, and
suggest changes in database "things" (queries, structure).

For the client I analyzed logs from production Pg instance for ~ 10
days.

During this time, the single most time consuming query (summarized time
for all instances of the query, with different parameters) was:

SELECT * FROM unapi.bre ( ... ) AS "unapi.bre";

I profiled this function, and found that in my test case most of the
time (2.04s out of 2.06s, so ~ 99%) was spent in call to
unapi.holdings_xml() function.

When I profiled this function, I found that most of the time (sorry,
don't have the number now with me) was spent in call to
evergreen.ranked_volumes() function.

At this moment in my research something changed on the server I was
testing on, and all subsequent times were ~ 4-5 times lower, but the
ratios were more or less the same.

Anyway - call to evergreen.ranked_volumes() showed repeatable time (with
full caches/buffers) of ~ 380ms.

I modified the function by:
1. inlining actor.org_unit_descendants(?, ?)
2. inlining evergreen.rank_ou(?, ?, ?)
3. extracting depth calculation to separate call
4. switched to plpgsql (which gives me ability to use variables)
5. removed evergreen.rank_ou() and evergreen.rank_cp_status() from
   select clause - these are still in WINDOW definition, but they
   weren't used in the SELECT, so it's better to remove from there.
6. in passing renamed arguments to avoid name clash (argument depth vs.
   field depth)
7. in passing changed usage of $* to access parameters to using named
   parameters, for readability.

New function did the same work in ~ 18ms.

Now - after I finished my tests, I was unfortunately not able to repeat
slow performance of unapi.bre(), but I think that the optimization of
evergreen.ranked_volumes() is good on its own.

If you'll agree, I would appreciate modifying the function in project.
If I should do/provide something else, please let me know.

Best regards,

depesz

NOTE: I (Jason Stephenson) have depesz's permission to create a git branch of his function changes with his name and email as the author. We both think this will get more attention if there is a branch and an associated Launchpad bug.

[Jason Stephenson (jstephenson)]

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

I'll add a pullrequest tag when I've had a chance to look at the above and signed off on it, myself.

[Kathy Lussier (klussier)]

This code has been on Jason's development server for well over a month now without any problems. I gave it closer scrutiny both on his server (running master) as well as another test server running 2.4.

My understanding is that unapi is used in the display for the search results page to verify that copies were retrieved in the expected sort order (preferred library -> search library name -> call number label -> availability) and that visibility checks were applied correctly. I did this search the consortium, individual branches and as a logged-in user with a preferred search library.

Functionally, it appears to be working as expected. I didn't do any before and after timed searches to note whether there was an improvement in performance, but there wasn't a notable decrease either.

[Dan Wells (dbw2)]

Added pullrequest to help move this along.

[Mike Rylander (mrylander)]

I've converted this back to an SQL-language function to reduce overhead and allow the possibility of planning improvements, as well as avoid a little schema churn caused by parameter renaming. It is meant to keep all the improvements from Depesz, and add a little in the way of a ROWS hint for the common case of returning only a few rows instead of the default 1000. See:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/miker/lp1234845_ranked_volumes

[Mike Rylander (mrylander)]

I'll rebase this in light of the recent changes to master. Updates soon!

[Mike Rylander (mrylander)]

Here's a rebased version: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/miker/lp1234845_ranked_volumes-rebased

[Kathy Lussier (klussier)]

Thanks Mike! I've done the same testing on this branch. The sort order for items on the search results page appears to be correct and the it looks like the proper visibility checks are being applied. I've signed off on this branch at

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

[Ben Shum (bshum)]

Going to review this, but I'd like to get sign-off / DCO (http://wiki.evergreen-ils.org/doku.php?id=dev:standing_dco) from depesz and Dyrcona who started work on this, further refined with eeevil and signed off by kmlussier. Just making sure we get all our ducks in a row before we commit it.

Sounds intriguing!

[Kathy Lussier (klussier)]

DCO from depesz at http://markmail.org/message/rv4vaarwixeswqgu

[Ben Shum (bshum)]

Excellent, updated the commit with all the notes and this has been pushed to master for 2.7

Thanks everyone!