Re: cache lookup failed from empty plpythonu function

On 2013-01-24 11:40:33 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2013-01-24 15:03:36 +0100, Sandro Santilli wrote:
> > ISTM the caching code for plpythonu trigger functions has been broken
> > for some time. The bug seem to be that PLy_procedure_get looks up the
> > function in a separate cache for trigger functions (PLy_trigger_cache)
> > but still only uses the function oid for lookup.
> > So the same function can be looked up for two tables and when the cached
> > entry originally refers to the old table that has been dropped we see
> > the above error.
>
> > The fix seems tob e to make PLy_trigger_cache have a key of (reloid,
> > fn_oid) instead of just fn_oid.
>
> If there's anything in either the cache entry itself or the function's
> other persistent state that is dependent on which table it's attached
> to, then yeah, obviously. But another conceivable solution would be to
> not store any such information.
>
> In a quick look, it seems like the "PLyTypeInfo result" field might
> depend on which table the trigger is attached to, so unless we can
> get along without that, we'll need to expand the hash key.
>
> Actually I'm wondering a bit why there are two hash tables at all.
> Seems like one table with a hash key of (fnoid, reloid) could be
> used, with the convention of reloid = 0 for non-trigger calls.

Its slightly more complex than just making it one hash table with an
extended key. When validating a trigger function we don't have a
relation to do the cache lookup. I chose to handle that case by not
doing a cache lookup at all in that case which imo is a sensible
choice.

I am less sure about whether its nicer to make that decision in
PLy_procedure_get as I have or whether it would have been better to make
PLy_procedure_create public and directly use it in plpython_validator.

The bug got introduced in 46211da1b84bc3537e799ee1126098e71c2428e8 which
interestingly says "Using HTABs instead of Python dictionaries makes
error recovery easier, and allows for procedures to be cached based on
their OIDs, not their names." - but the caching seems to always have
used ("%u_%u", fn_oid, tgreloid) as its key which explains why the bug
didn't exist back then.

Anyway, as far as I can see only 9.1 upwards are affected.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services