Обсуждение: 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.

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! :)

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