Hold tests failure

[Dan Scott]

* PostgreSQL 9.3
* Fedora 20
* Evergreen master as of 2014-02-07

Trying to place a hold through the TPAC using the admin user and email notification results in a server error; the logs contain the following:

[SELECT * FROM action.hold_request_permit_test( '8', '1', '2905', '1', '1' ) AS "action.hold_request_permit_test" ;]: 0 ERROR: COLUMN reference "pickup_ou" IS ambiguous
LINE 5: ... LEFT JOIN actor.org_unit_ancestors_distance( pickup_ou ...
                                                             ^
DETAIL: It could refer TO either a PL/pgSQL variable OR a TABLE COLUMN.

So it looks like hold placement is entirely broken in master.

On the bright side, it should be pretty easy to create a pgTAP test for this...

[Dan Wells (dbw2)]

There isn't a lot to go on in our codebase, but it looks like in at least a few similar cases we prefixed the variable with 'v_' to work around this sort of ambiguity. Unless somebody feels strongly that we should take this as an opportunity to establish some other naming convention, that works for me.

[Mike Rylander (mrylander)]

Dan,

I've used v_ or similar elsewhere in our stored procs, and it makes sense to me. We could declare that all variables must be named with strict Hungarian notation, but ... no. ;)

+1

[Mike Rylander (mrylander)]

Oh, and to avoid the need to drop functions because of parameter name changes, we can use that "newname ALIAS FOR oldname;" syntax in the DECLARE section of the function.

[Mike Rylander (mrylander)]

Here is a branch that attempts to do just this: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/miker/lp1277731-hold-test-failure-pg_93

[Dan Wells (dbw2)]

Just for my own learning, we are using "ALIAS FOR" rather than change the parameter names directly just in case these are being called with named arguments somewhere? (As far as I know, we're not doing that (in these cases at least).) Or is there something more to it than that? I am trying to fully understand the benefits for possible future need. Thanks!

[Mike Rylander (mrylander)]

I used ALIAS FOR because if you change parameter names then you have to DROP (rather than CREATE OR REPLACE) the function. I try to avoid that when possible because we /can/ call functions with parameter names, and if (some time in the future) Postgres gets dependency tracking features for the guts of functions, the function signature will surely be the first thing it checks -- meaning a "Can't replace. HINT: drop cascade" type message.

The second is hand-wavy, and perhaps it's unwarranted? If you or others think so, I would be perfectly fine with an implementation that just changes the parameter name.

[Dan Wells (dbw2)]

This branch worked fine for me, pushed to master with pgtap test coming momentarily.

Thanks, Mike!

[Dan Wells (dbw2)]

And... pgTAP test added.

[Rogan Hamby (rogan-hamby)]

Tested and worked fine for me.