Re: cache lookup failed from empty plpythonu function

On 2013-01-25 17:07:59 -0500, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > 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.
>
> Patch applied with some editorialization, mostly cosmetic but one thing
> not so much: you introduced uninitialized-variable hazards into
> PLy_procedure_get, since if use_cache is false then found, entry, and
> proc wouldn't get set up.

Gah. Now I am pissed at myself. I thought I had initialized 'found' to
false which should be enough.

> Neither version of gcc I tried complained about this, and I assume yours
> didn't either, which is a bit annoying/scary. I wonder whether the
> "volatile" marker prevents that?

I think the control flow is just to complex for gcc, it probably cannot
trace anything across a setjmp() althout it theoretically should be
possible given the failure is in the branch that is executed
unconditionally.
Given that the real culprit is 'found' I don't think its the volatiles.

> BTW, it seems to me that there is another bug in PLy_procedure_get,
> which is that it's not thinking about recursive use of a procedure.
> Consider
>
> call f
>
> ... somebody updates f's pg_proc tuple
>
> ... recursively call f
>
> At this point PLy_procedure_get will decide the cache entry is obsolete
> and happily trash the procedure data that the outer call is still using.
> We ran into this long ago with plpgsql and fixed it by adding
> reference-counting logic. Probably the same needs to be done here.
>
> There are probably not that many people trying to use plpython functions
> in such a way (note the recursion would have to happen via a SPI query,
> not directly within Python). But still...

I am afraid youre right. It seems ugly that all pl's have to reinvent
that kind of cache + invalidation logic. I am e.g. not sure they go far
enough in invalidating stuff like parameter types & io functions.

Greetings,

Andres Freund

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