On Mon, Nov 1, 2010 at 09:28, Tom Lane <[email protected]> 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?
*** a/src/pl/tcl/pltcl.c
--- b/src/pl/tcl/pltcl.c
***************
*** 137,143 **** typedef struct pltcl_query_desc
/**********************************************************************
* For speedy lookup, we maintain a hash table mapping from
! * function OID + trigger OID + user OID to pltcl_proc_desc pointers.
* The reason the pltcl_proc_desc struct isn't directly part of the hash
* entry is to simplify recovery from errors during compile_pltcl_function.
*
--- 137,143 ----
/**********************************************************************
* For speedy lookup, we maintain a hash table mapping from
! * function OID + trigger flag + user OID to pltcl_proc_desc pointers.
* The reason the pltcl_proc_desc struct isn't directly part of the hash
* entry is to simplify recovery from errors during compile_pltcl_function.
*
***************
*** 149,155 **** typedef struct pltcl_query_desc
typedef struct pltcl_proc_key
{
Oid proc_id; /* Function OID */
! Oid trig_id; /* Trigger OID, or 0 if not trigger */
Oid user_id; /* User calling the function, or 0 */
} pltcl_proc_key;
--- 149,159 ----
typedef struct pltcl_proc_key
{
Oid proc_id; /* Function OID */
! /*
! * is_trigger is really a bool, but declare as Oid to ensure this struct
! * contains no padding
! */
! Oid is_trigger; /* is it a trigger function? */
Oid user_id; /* User calling the function, or 0 */
} pltcl_proc_key;
***************
*** 1172,1178 **** compile_pltcl_function(Oid fn_oid, Oid tgreloid, bool pltrusted)
/* Try to find function in pltcl_proc_htab */
proc_key.proc_id = fn_oid;
! proc_key.trig_id = tgreloid;
proc_key.user_id = pltrusted ? GetUserId() : InvalidOid;
proc_ptr = hash_search(pltcl_proc_htab, &proc_key,
--- 1176,1182 ----
/* Try to find function in pltcl_proc_htab */
proc_key.proc_id = fn_oid;
! proc_key.is_trigger = OidIsValid(tgreloid);
proc_key.user_id = pltrusted ? GetUserId() : InvalidOid;
proc_ptr = hash_search(pltcl_proc_htab, &proc_key,
***************
*** 1228,1241 **** compile_pltcl_function(Oid fn_oid, Oid tgreloid, bool pltrusted)
int tcl_rc;
/************************************************************
! * Build our internal proc name from the functions Oid + trigger Oid
************************************************************/
! if (!is_trigger)
! snprintf(internal_proname, sizeof(internal_proname),
! "__PLTcl_proc_%u", fn_oid);
! else
! snprintf(internal_proname, sizeof(internal_proname),
! "__PLTcl_proc_%u_trigger_%u", fn_oid, tgreloid);
/************************************************************
* Allocate a new procedure description block
--- 1232,1241 ----
int tcl_rc;
/************************************************************
! * Build our internal proc name
************************************************************/
! snprintf(internal_proname, sizeof(internal_proname),
! "__PLTcl_proc_%u", fn_oid);
/************************************************************
* Allocate a new procedure description block
--
Sent via pgsql-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
