Re: PG 8.1beta3 out soon

Core's current plan is to bundle 8.1beta3 tomorrow evening (Tuesday PM,
North American east coast time) for announcement Wednesday. Any last
minute bug fixes out there?

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> Core's current plan is to bundle 8.1beta3 tomorrow evening (Tuesday PM,
> North American east coast time) for announcement Wednesday. Any last
> minute bug fixes out there?

Anyone able to duplicate my plperl bug? If it is genuine, I would really
like to see it fixed for 8.1, as it's a showstopper.

- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200510101855
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFDSvFvvJuQZxSWSsgRAuF/AJ9NFETn9KyTjjZh5qKLLyESMZkAgQCgpPHf
lc4taOTI7maMmfTgkDjtIDs=
=rLzu
-----END PGP SIGNATURE-----

Greg Sabino Mullane wrote:

>>Core's current plan is to bundle 8.1beta3 tomorrow evening (Tuesday PM,
>>North American east coast time) for announcement Wednesday. Any last
>>minute bug fixes out there?
>>
>>
>
>Anyone able to duplicate my plperl bug? If it is genuine, I would really
>like to see it fixed for 8.1, as it's a showstopper.
>
>
>
>

I take it you are referring to this:
http://archives.postgresql.org/pgsql-bugs/2005-10/msg00095.php

I don't think it's really a bug - it's a well known perl effect that has
caught many people over the years, especially unwary users of
Apache::Registry who fail to recognise that their scripts are being
wrapped inside an anonymous subroutine.

I took your example and simulated it entirely outside postgres/plperl to
show that this is a pure perl effect. The script is like this:

---------------------------------------------------
#!/usr/bin/perl

use strict;
use warnings;

use constant { INFO => 'INFO' };

sub elog
{
print join(": ",@_),"\n";
}

my $func = sub
{

my $_TD = $_[0]; shift;
# use vars qw($_TD); local $_TD = shift;

my $event = $_TD->{event};
elog(INFO, "Top event : $event");
my $newname = $_TD->{new}{a};
elog(INFO, "Top newname : $newname");
&subber($event);

sub subber
{
no warnings qw(uninitialized);
my $arg = shift;
elog(INFO, join(" | ",caller(0)));
elog(INFO, join(" | ",caller(1)));
elog(INFO, "Sub global : $event");
elog(INFO, "Sub direct : $_TD->{event}");
my $newname = $_TD->{new}{a};
elog(INFO, "Sub newname : $newname");
}

elog(INFO, "Bottom event : $event");
};

&$func({ new=>{a=>22},event=>'INSERT' });
&$func({ new=>{a=>33},event=>'UPDATE' });
&$func({ new=>{a=>44},event=>'INSERT' });
&$func({ new=>{a=>55},event=>'UPDATE' });

--------------------------------------------------

and the output is this - not the telltale first two lines:

[andrew(at)alphonso ~]$ perl nonanontry.pl
Variable "$event" will not stay shared at nonanontry.pl line 34.
Variable "$_TD" will not stay shared at nonanontry.pl line 35.
INFO: Top event : INSERT
INFO: Top newname : 22
INFO: main | nonanontry.pl | 26 | main::subber | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: main | nonanontry.pl | 43 | main::__ANON__ | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: Sub global : INSERT
INFO: Sub direct : INSERT
INFO: Sub newname : 22
INFO: Bottom event : INSERT
INFO: Top event : UPDATE
INFO: Top newname : 33
INFO: main | nonanontry.pl | 26 | main::subber | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: main | nonanontry.pl | 44 | main::__ANON__ | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: Sub global : INSERT
INFO: Sub direct : INSERT
INFO: Sub newname : 22
INFO: Bottom event : UPDATE
INFO: Top event : INSERT
INFO: Top newname : 44
INFO: main | nonanontry.pl | 26 | main::subber | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: main | nonanontry.pl | 45 | main::__ANON__ | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: Sub global : INSERT
INFO: Sub direct : INSERT
INFO: Sub newname : 22
INFO: Bottom event : INSERT
INFO: Top event : UPDATE
INFO: Top newname : 55
INFO: main | nonanontry.pl | 26 | main::subber | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: main | nonanontry.pl | 46 | main::__ANON__ | 1 | | | | 2 |
UUUUUUUUUUUU
INFO: Sub global : INSERT
INFO: Sub direct : INSERT
INFO: Sub newname : 22
INFO: Bottom event : UPDATE
[andrew(at)alphonso ~]$

Now, if we swap the line that declares $_TD (which is just like it is in
plperl.c) for the line underneath it, we could get rid of this effect
(try it and see). I don't think it would have any memory leaks, but we'd
need to check.

However, that would only avoid the problem for what we know about,
namely $_TD. The problem would remain for any lexical variable, because
you are using a named nested subroutine which tries to access the
lexical in its declaratory scope. Pass the hashref as an argument and
have it only refer to the passed object rather than the one that is
lexically visible and all should be well.

My take: we should document this better, but it ain't broke so it don't
need fixing, although I would not object strenuously to changing the
behaviour of $_TD.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> My take: we should document this better, but it ain't broke so it don't
> need fixing,

Actually, my take on your analysis is that there should be a way to get
at "use warnings" (I assume that's disallowed in trusted plperl).

regards, tom lane

> Core's current plan is to bundle 8.1beta3 tomorrow evening (Tuesday PM,
> North American east coast time) for announcement Wednesday. Any last
> minute bug fixes out there?

Not a bug fix, but this bug still hasn't been looked at:

http://archives.postgresql.org/pgsql-hackers/2005-04/msg00499.php

Chris

Tom Lane wrote:

>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>
>>My take: we should document this better, but it ain't broke so it don't
>>need fixing,
>>
>>
>
>Actually, my take on your analysis is that there should be a way to get
>at "use warnings" (I assume that's disallowed in trusted plperl).
>
>
>
>

Yes, we can't allow "use" in trusted code. But we could turn it on in
plperl.c, just as we can turn on strict mode, and HEAD already has the
infrastructure for logging lexical warnings - that's a new feature. I
will investigate turning the switch.

cheers

andrew

Christopher Kings-Lynne <chriskl(at)familyhealth(dot)com(dot)au> writes:
> Not a bug fix, but this bug still hasn't been looked at:
> http://archives.postgresql.org/pgsql-hackers/2005-04/msg00499.php

I'm not really convinced that's a bug, and in any case it's not going to
be dealt with in 8.1.

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
NotDashEscaped: You need GnuPG to verify this message

> I don't think it's really a bug - it's a well known perl effect that has
> caught many people over the years, especially unwary users of
> Apache::Registry who fail to recognise that their scripts are being
> wrapped inside an anonymous subroutine.

Ah, ok, so it's a documentation bug! :)

> My take: we should document this better, but it ain't broke so it don't
> need fixing, although I would not object strenuously to changing the
> behaviour of $_TD.

Hmm...what if we did this?:

Index: plperl.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/pl/plperl/plperl.c,v
retrieving revision 1.92
diff -r1.92 plperl.c
671c671
< XPUSHs(sv_2mortal(newSVpv("my $_TD=$_[0]; shift;", 0)));
---
> XPUSHs(sv_2mortal(newSVpv("our $_TD=$_[0]; shift;", 0)));

--
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200510110838
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8
-----BEGIN PGP SIGNATURE-----

iD8DBQFDS7J2vJuQZxSWSsgRAt7XAKC8D7HohA27CnaD7SVLrdKi80K49wCeI+o6
Tg9r3CSUeIV4872xuhZ0WBA=
=JRAX
-----END PGP SIGNATURE-----

Andrew Dunstan wrote:

> Tom Lane wrote:
>
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>
>>> My take: we should document this better, but it ain't broke so it
>>> don't need fixing,
>>>
>> Actually, my take on your analysis is that there should be a way to get
>> at "use warnings" (I assume that's disallowed in trusted plperl).
>
> Yes, we can't allow "use" in trusted code. But we could turn it on in
> plperl.c, just as we can turn on strict mode, and HEAD already has the
> infrastructure for logging lexical warnings - that's a new feature. I
> will investigate turning the switch.
>

I spoke a bit rashly here. The only way I have been able to make it work
so far in the Safe container is via the global -w flag - everything else
I tried failed, although it worked just fine for untrusted code. I don't
have lots of time to spend working out why. Another issue is that the
warnings pragma is fairly recent, and so might break on older perls
anyway, so just using -w might be the best way to go, if we do anything.
However, this turns on all warnings (e.g. use of uninitialized
variables) and the user can't turn them off. Still, that might not be a
bad thing. It will just cause the warnings to be logged, although
possibly a little verbosely.

That change at least is trivial.

So what's the consensus? "-w" or just document?

FYI, here is the perldiag man page extract covering the problem:

Variable "%s" will not stay shared
(W closure) An inner (nested) named subroutine is referencing a
lexical variable defined in an outer subroutine.

When the inner subroutine is called, it will probably see the value
of the outer subroutine’s variable as it was before and during the
*first* call to the outer subroutine; in this case, after the first
call to the outer subroutine is complete, the inner and outer sub-
routines will no longer share a common value for the variable. In
other words, the variable will no longer be shared.

Furthermore, if the outer subroutine is anonymous and references a
lexical variable outside itself, then the outer and inner subrou-
tines will never share the given variable.

This problem can usually be solved by making the inner subroutine
anonymous, using the "sub {}" syntax. When inner anonymous subs
that reference variables in outer subroutines are called or refer-
enced, they are automatically rebound to the current values of such
variables.

cheers

andrew

Greg Sabino Mullane wrote:

>
>Hmm...what if we did this?:
>
>Index: plperl.c
>===================================================================
>RCS file: /projects/cvsroot/pgsql/src/pl/plperl/plperl.c,v
>retrieving revision 1.92
>diff -r1.92 plperl.c
>671c671
>< XPUSHs(sv_2mortal(newSVpv("my $_TD=$_[0]; shift;", 0)));
>---
>
>
>> XPUSHs(sv_2mortal(newSVpv("our $_TD=$_[0]; shift;", 0)));
>>
>>
>
>
>
>

That would probably work, but it would ONLY deal with the issue for
$_TD. In your function $event will still hit this problem.

see next email for sidcussion of warnings.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>> Actually, my take on your analysis is that there should be a way to get
>>> at "use warnings" (I assume that's disallowed in trusted plperl).
>>
>> Yes, we can't allow "use" in trusted code. But we could turn it on in
>> plperl.c, just as we can turn on strict mode, and HEAD already has the
>> infrastructure for logging lexical warnings - that's a new feature. I
>> will investigate turning the switch.

> I spoke a bit rashly here. The only way I have been able to make it work
> so far in the Safe container is via the global -w flag - everything else
> I tried failed, although it worked just fine for untrusted code. I don't
> have lots of time to spend working out why.

I think we'd best put this on the TODO list for 8.2. It's awfully late
in the cycle to be rushing through half-thought-out features ...

regards, tom lane

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

> That would probably work, but it would ONLY deal with the issue for
> $_TD. In your function $event will still hit this problem.

Well, fixing $_TD would pretty much fix all the problems I've been having.
As far as "$event", that is in my control and easily fixed by making it
"our" as well. Some explicit warnings and best practices in the docs would
be nice.

Tom, can we possibly get that one-word patch into 8.1? It would go a long
way towards making plperl more intuitive (and useful), and probably head
off some future "bug" reports.

Thanks,
- --
Greg Sabino Mullane greg(at)turnstep(dot)com
PGP Key: 0x14964AC8 200510110941
http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8

-----BEGIN PGP SIGNATURE-----

iD8DBQFDS8E4vJuQZxSWSsgRAvsYAJ9cz6yNdInOY2HbGJ5LKKBRFxe97QCgwoa4
r4sJrmQ4cHPE+NyYo+9dX1A=
=Kfwb
-----END PGP SIGNATURE-----

On Tue, Oct 11, 2005 at 08:51:01AM -0400, Andrew Dunstan wrote:
>
>
> Andrew Dunstan wrote:
>
> >Tom Lane wrote:
> >
> >>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >>
> >>>My take: we should document this better, but it ain't broke so it
> >>>don't need fixing,
> >>>
> >>Actually, my take on your analysis is that there should be a way
> >>to get at "use warnings" (I assume that's disallowed in trusted
> >>plperl).
> >
> >Yes, we can't allow "use" in trusted code. But we could turn it on
> >in plperl.c, just as we can turn on strict mode, and HEAD already
> >has the infrastructure for logging lexical warnings - that's a new
> >feature. I will investigate turning the switch.
> >
>
> I spoke a bit rashly here. The only way I have been able to make it
> work so far in the Safe container is via the global -w flag -
> everything else I tried failed, although it worked just fine for
> untrusted code. I don't have lots of time to spend working out why.
> Another issue is that the warnings pragma is fairly recent, and so
> might break on older perls anyway, so just using -w might be the
> best way to go, if we do anything. However, this turns on all
> warnings (e.g. use of uninitialized variables) and the user can't
> turn them off. Still, that might not be a bad thing. It will just
> cause the warnings to be logged, although possibly a little
> verbosely.
>
> That change at least is trivial.
>
> So what's the consensus? "-w" or just document?

+1 for -w. Documenting wouldn't hurt either.

Cheers,
D
--
David Fetter david(at)fetter(dot)org http://fetter.org/
phone: +1 510 893 6100 mobile: +1 415 235 3778

Remember to vote!

Greg Sabino Mullane wrote:

>>That would probably work, but it would ONLY deal with the issue for
>>$_TD. In your function $event will still hit this problem.
>>
>>
>
>Well, fixing $_TD would pretty much fix all the problems I've been having.
>As far as "$event", that is in my control and easily fixed by making it
>"our" as well. Some explicit warnings and best practices in the docs would
>be nice.
>
>Tom, can we possibly get that one-word patch into 8.1? It would go a long
>way towards making plperl more intuitive (and useful), and probably head
>off some future "bug" reports.
>
>

"our" only came in with Perl 5.6 ... I don't recall if we have declared
support for earlier versions than that dead yet, although David Fetter
and Joshua Drake have urged us to.

I will add a note somewhere in the docs in the next few days advising
against using named subroutines that refer to lexical variables from the
enclosing scope.

cheers

andrew