Thread: Plperl trigger variables no longer global
This works in 9.0 but not in git/9.1 beta: CREATE FUNCTION wheredidmytdgo() RETURNS TRIGGER LANGUAGE plperlu AS $bc$ use strict; use warnings; my $new =3D $_TD->{new}; return; $bc$; The error is: ERROR: Global symbol "$_TD" requires explicit package name at line 3. CONTEXT: compilation of PL/Perl function "wheredidmytdgo" --=20 Greg Sabino Mullane greg@endpoint.com End Point Corporation PGP Key: 0x14964AC8
On Tue, May 3, 2011 at 17:40, Greg Sabino Mullane <greg@endpoint.com> wrote: > This works in 9.0 but not in git/9.1 beta: > > CREATE FUNCTION wheredidmytdgo() > RETURNS TRIGGER > LANGUAGE plperlu > AS > $bc$ > =C2=A0 =C2=A0use strict; use warnings; > =C2=A0 =C2=A0my $new =3D $_TD->{new}; > =C2=A0 =C2=A0return; > $bc$; > > The error is: > > ERROR: =C2=A0Global symbol "$_TD" requires explicit package name at line = 3. > CONTEXT: =C2=A0compilation of PL/Perl function "wheredidmytdgo" This seems to be broken by http://git.postgresql.org/gitweb?p=3Dpostgresql.git;a=3Dcommit;h=3Def19dc6d= 39dd2490ff61489da55d95d6941140bf (Set up PLPerl trigger data using C code instead of Perl code.) Im not sure what the right fix is. Copying what plperl_call_trigger_func() does for _TD ("get_sv("_TD", GV_ADD); ..." into plperl_create_sub() does not seem to work.
On Wed, May 4, 2011 at 16:20, Alex Hunsaker <badalex@gmail.com> wrote: > > This seems to be broken by > http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=ef19dc6d39dd2490ff61489da55d95d6941140bf > (Set up PLPerl trigger data using C code instead of Perl code.) > > Im not sure what the right fix is. Copying what > plperl_call_trigger_func() does for _TD ("get_sv("_TD", GV_ADD); ..." > into plperl_create_sub() does not seem to work. Just a small clarification, this only breaks when running under use strict;. After playing with it a bit more I see 2 clear options: 1) make $_TD global like %_SHARED. This should not cause any problems as we make $_TD private via local() before each trigger call. Also pre 9.1 non trigger functions could still access and check the definedness of $_TD so if someone was relying on that (for whatever unknown reason) that will work again. 2) revert the optimization #1 is very small and I don't see any downsides (maybe we should local $_TD before regular perl calls as well??). Find it attached.
Attachment
Excerpts from Alex Hunsaker's message of mié may 04 23:53:34 -0300 2011: > After playing with it a bit more I see 2 clear options: > 1) make $_TD global like %_SHARED. This should not cause any problems > as we make $_TD private via local() before each trigger call. Also pre > 9.1 non trigger functions could still access and check the definedness > of $_TD so if someone was relying on that (for whatever unknown > reason) that will work again. This is strange. Are you saying that there's no decent way to make a variable global in C code? (In other words, the Perl code to create the global var is indecently implemented) -- Ãlvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thu, May 5, 2011 at 06:51, Alvaro Herrera <alvherre@commandprompt.com> wrote: > Excerpts from Alex Hunsaker's message of mié may 04 23:53:34 -0300 2011: > >> After playing with it a bit more I see 2 clear options: >> 1) make $_TD global like %_SHARED. This should not cause any problems >> as we make $_TD private via local() before each trigger call. Also pre >> 9.1 non trigger functions could still access and check the definedness >> of $_TD so if someone was relying on that (for whatever unknown >> reason) that will work again. > > This is strange. Are you saying that there's no decent way to make a > variable global in C code? Im sure we could... I don't see any reason to do it in C. (performance or otherwise) In other news I found another bug with this-- it was trying to local($_TD) by using SAVESPTR() when it seems it really should be using save_item(). Currently its not really localizing $_TD, which at the very least means recursive triggers might modify the callers $_TD. Ugh. Fixed in the attached plus added regression tests for both issues (use strict; && Global symbol "$_TD" requires explicit package name, test recursive trigger calls). Although Ill admit, given the point we are in the release I could see a revert also being justified. Greg, big thanks for testing! keep it up! :)
Attachment
On Thu, May 5, 2011 at 12:14 PM, Alex Hunsaker <badalex@gmail.com> wrote: > On Thu, May 5, 2011 at 06:51, Alvaro Herrera <alvherre@commandprompt.com>= wrote: >> Excerpts from Alex Hunsaker's message of mi=E9 may 04 23:53:34 -0300 201= 1: >> >>> After playing with it a bit more I see 2 clear options: >>> 1) make $_TD global like %_SHARED. This should not cause any problems >>> as we make $_TD private via local() before each trigger call. Also pre >>> 9.1 non trigger functions could still access and check the definedness >>> of $_TD so if someone was relying on that (for whatever unknown >>> reason) that will work again. >> >> This is strange. =A0Are you saying that there's no decent way to make a >> variable global in C code? > > Im sure we could... I don't see any reason to do it in C. (performance > or otherwise) > > In other news I found another bug with this-- it was trying to > local($_TD) by using SAVESPTR() when it seems it really should be > using save_item(). Currently its not really localizing $_TD, which at > the very least means recursive triggers might modify the callers $_TD. > Ugh. > > Fixed in the attached plus added regression tests for both issues (use > strict; && Global symbol "$_TD" requires explicit package name, test > recursive trigger calls). Although Ill admit, given the point we are > in the release I could see a revert also being justified. > > Greg, big thanks for testing! keep it up! :) Do we need to apply this patch? --=20 Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, May 15, 2011 at 14:02, Robert Haas <robertmhaas@gmail.com> wrote: >> Fixed in the attached plus added regression tests for both issues (use >> strict; && Global symbol "$_TD" requires explicit package name, test >> recursive trigger calls). Although Ill admit, given the point we are >> in the release I could see a revert also being justified. >> >> Greg, big thanks for testing! keep it up! :) > > Do we need to apply this patch? Only if we want use strict and trigger functions to work in 9.1 :). I realize I did a poor job of explaining the problem and the solution, that probably made it hard for a -commiter to pick up. Here is a stab at that. [ pgcon is spinning up so I won't hold my breath waiting for a response ] Problem: CREATE FUNCTION wheredidmytdgo() RETURNS TRIGGER LANGUAGE plperlu AS $bc$ use strict; my $new = $_TD->{new}; return; $bc$; fails with ERROR: Global symbol "$_TD" requires explicit package name at line 3. Analysis: The above fails due to http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=ef19dc6d39dd2490ff61489da55d95d6941140bf. It moved declaring of $_TD from plperl_create_sub to plperl_call_perl_trigger_func(). Before it was always declared even for non trigger functions. This broke CREATE FUNCTION validation as $_TD has not been declared. Assuming you could get past the validator things would still not work, plperl_call_trigger_func() tried to declare it using get_sv("_TD", GV_ADD) which seems like it should work, but does not[1]. We (I) failed to notice this during review as it only shows when running under "use strict", which apparently is uncommon. Fix: My proposed fix is instead of declaring $_TD in plperl_call_perl_trigger_func(), just declare it as a global the same way we declare %_SHARED. It keeps things simple and we should get to keep any performance benefits from the patch. While playing with it I also noticed it was failing to be local()ized properly meaning nested trigger functions would clobber $_TD, also fix that. Also added regression tests for both issues. -- (1) If it was working and creating a global correctly you should be able to call a trigger function so that $_TD gets declared and then create a new trigger function that uses strict and references $_TD without any problems. But it doesn't work... # create or replace function td() returns trigger language plperlu as $bc$ return; $bc$; CREATE FUNCTION # create table trig_test(a int); CREATE TABLE # create trigger test_trig after insert on trig_test execute procedure td(); CREATE TRIGGER # insert into trig_test values (1); INSERT 0 1 # CREATE or replace FUNCTION wheredidmytdgo() RETURNS TRIGGER LANGUAGE plperlu AS $bc$ use strict; my $new = $_TD->{new}; return; $bc$; ERROR: Global symbol "$_TD" requires explicit package name at line 3.
On Mon, May 16, 2011 at 12:57:41PM -0600, Alex Hunsaker wrote: > > Do we need to apply this patch? ... > My proposed fix is instead of declaring $_TD in Yes, please apply, I'm eager to continue testing 9.1 but cannot=20 proceed until something is in place. --=20 Greg Sabino Mullane greg@endpoint.com End Point Corporation PGP Key: 0x14964AC8
Excerpts from Greg Sabino Mullane's message of mié may 18 12:03:00 -0400 2011: > On Mon, May 16, 2011 at 12:57:41PM -0600, Alex Hunsaker wrote: > > > Do we need to apply this patch? > ... > > My proposed fix is instead of declaring $_TD in > > Yes, please apply, I'm eager to continue testing 9.1 but cannot > proceed until something is in place. Done, please continue testing. -- Ãlvaro Herrera <alvherre@commandprompt.com> The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support