On Mon, Nov 1, 2010 at 09:28, Tom Lane <t...@sss.pgh.pa.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?
*** 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 (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers