Re: why does plperl cache functions using just a bool for is_trigger

Andrew Dunstan <andrew@dunslane.net> writes:
> On 10/31/2010 04:40 PM, Alex Hunsaker wrote:
>> which happens because prodesc->result_in_func.fn_addr (flinfo) is
>> NULL.  That happens because when we are a trigger we don't setup
>> input/output conversion.  And with the change we get the same
>> proc_desc for triggers and non triggers, so if the trigger function
>> gets called first, any call to the direct function will use the same
>> proc_desc with the wrong input/out conversion.

> How does that happen given that the function Oid is part of the hash key?

I think the crash is dependent on the fact that the function is created
and called in the same session.  That means the validator gets called on
it first, and the validator not unreasonably assumes istrigger = true,
and then it calls compile_plperl_function which sets up a cache entry
on that basis.  So then when the "regular" call comes along, it tries
to reuse the cache entry in the other style.  Kaboom.

>> There is a check so that trigger functions can not be called as plain
>> functions, but it only gets called when we do not have an entry in
>> plperl_proc_hash.  I think just moving that up, something the like the
>> attached should be enough.

> This looks like the right fix, though.

No, that is just moving a test that only needs to be done once into a
place where it has to be done every time.  You might as well argue that
we shouldn't cache any of the setup but just redo it all every time.

The fundamental issue here is that the contents of plperl_proc_desc
structs are different between the trigger and non-trigger cases.
Unless you're prepared to make them the same, and guarantee that they
always will be the same in future, I think that including the istrigger
flag in the hash key is a good safety feature.  It's also the same way
that the other three PLs do things, and I see no good excuse for plperl
to do things differently here.

IOW, it's not broke, let's not fix it.
        regards, tom lane