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

On Mon, Nov 1, 2010 at 09:28, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> 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.

The other Kaboom happens if the trigger gets called as a trigger first
and then directly.

>>> There is a check so that trigger functions can not be called as plain
>>> functions... I think just moving that up...

> 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.

Huh? I might try and argue that if the new test was more complex than
2 compares :P. In-fact the way it stands now we uselessly grab the
functions pg_proc entry in the common case. Would you object to
trying to clean that up across all pls? Im thinking add an is_trigger
or context to each proc_desc, then check that in the corresponding
handlers. While im at it get rid of at least one SysCache lookup.
Thoughts? We can still keep the is_trigger bool in the hash key, as
you said below it is a good safety feature. I just wish the logic was
spelled out a bit more. Maybe im alone here.

> 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.

Im all in favor of keeping things between the pls as close as possible.

Speaking of which, pltcl stores the trigger reloid instead of a flag
(it also uses tg_reloid in the internal proname). It seems a tad
excessive to have one function *per* trigger table. I looked through
the history to see if there was some reason, it goes all the way back
to the initial commit. I assume its this way because it copied
plpgsql, which needs it as the rowtype might be different per table.
pltcl should not have that issue. Find attached a patch to clean that
up and make it match the other pls (err um plperl). It passes its
regression tests and some additional limited testing. Thoughts?