Re: PL/Perl function naming

Why do the release notes say this, under plperl:

* PL/Perl subroutines are now given names (Tim Bunce)
This is for the use of profiling and code coverage tools. DIDN'T
THEY HAVE NAMES BEFORE?

No they didn't, from perl's perspective, which is what this is about.
They had names from Postgres' POV, but those names were and are
invisible to perl. PL/Perl functions are (references to) anonymous
subroutines. If you say in perl:

my $foo = sub { $bar = 1; };

the subroutine has no name. The only thing that has a name is the
reference to it, which is quite a different thing. PL/Perl functions are
created using this sort of mechanism. The change that has been made is
to decorate them with names that can be used by profilers etc., although
the names can't be used to call them directly, IIRC.

If whoever put this in the release notes had bothered to ask I am sure
we would have been happy to explain.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> Why do the release notes say this, under plperl:
> * PL/Perl subroutines are now given names (Tim Bunce)
> This is for the use of profiling and code coverage tools. DIDN'T
> THEY HAVE NAMES BEFORE?

> If whoever put this in the release notes had bothered to ask I am sure
> we would have been happy to explain.

You don't need to complain, just fix it. The point of the comment is
that more explanation is needed.

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> Why do the release notes say this, under plperl:
>> * PL/Perl subroutines are now given names (Tim Bunce)
>> This is for the use of profiling and code coverage tools. DIDN'T
>> THEY HAVE NAMES BEFORE?
>>
>
>
>> If whoever put this in the release notes had bothered to ask I am sure
>> we would have been happy to explain.
>>
>
> You don't need to complain, just fix it. The point of the comment is
> that more explanation is needed.
>
>
>

OK ... I guess I was bothered because this has been referred to in a
public press release about the Beta. The PLPerl security stuff is
missing too, so I'll fix that also.

cheers

andrew

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> OK ... I guess I was bothered because this has been referred to in a
> public press release about the Beta. The PLPerl security stuff is
> missing too, so I'll fix that also.

The security stuff isn't relevant to the 9.0 notes, since it's already
been fixed in a previous release. In general we don't document bug
fixes in a major release if they already appeared in previous
back-patches; the major release notes are quite verbose enough without
such duplication. (In effect, the idea is that major release notes
should represent the delta from the previous major release *as it stood
at the time of the new major release*.)

regards, tom lane

Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>
>> OK ... I guess I was bothered because this has been referred to in a
>> public press release about the Beta. The PLPerl security stuff is
>> missing too, so I'll fix that also.
>>
>
> The security stuff isn't relevant to the 9.0 notes, since it's already
> been fixed in a previous release. In general we don't document bug
> fixes in a major release if they already appeared in previous
> back-patches; the major release notes are quite verbose enough without
> such duplication. (In effect, the idea is that major release notes
> should represent the delta from the previous major release *as it stood
> at the time of the new major release*.)
>

OK, then I will just remove the now redundant item regarding Safe.pm.

cheers

andrew

On Mon, May 17, 2010 at 11:34:37AM -0400, Tom Lane wrote:
> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> > Why do the release notes say this, under plperl:
> > * PL/Perl subroutines are now given names (Tim Bunce)
> > This is for the use of profiling and code coverage tools. DIDN'T
> > THEY HAVE NAMES BEFORE?
>
> > If whoever put this in the release notes had bothered to ask I am sure
> > we would have been happy to explain.
>
> You don't need to complain, just fix it. The point of the comment is
> that more explanation is needed.

I think you can just delete it. It's too esoteric to be worth noting.

Tim.

p.s. It also turned to be insufficiently useful for NYTProf since it
doesn't also update some internals to record the 'filename' and line
number range of the sub. So PostgreSQL::PLPerl::NYTProf works around
that by wrapping mkfuncsrc() to edit the generated perl code to include
a subname in the usual "sub $name { ... }" style. I may offer a patch
for 9.1 to to make it work that way.

Andrew Dunstan wrote:
>
>
> Tom Lane wrote:
> > Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >
> >> OK ... I guess I was bothered because this has been referred to in a
> >> public press release about the Beta. The PLPerl security stuff is
> >> missing too, so I'll fix that also.
> >>
> >
> > The security stuff isn't relevant to the 9.0 notes, since it's already
> > been fixed in a previous release. In general we don't document bug
> > fixes in a major release if they already appeared in previous
> > back-patches; the major release notes are quite verbose enough without
> > such duplication. (In effect, the idea is that major release notes
> > should represent the delta from the previous major release *as it stood
> > at the time of the new major release*.)
> >
>
>
> OK, then I will just remove the now redundant item regarding Safe.pm.

Yes, please make whatever updates to the release notes you feel are
appropriate.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

Tim Bunce wrote:
> On Mon, May 17, 2010 at 11:34:37AM -0400, Tom Lane wrote:
>
>> Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
>>
>>> Why do the release notes say this, under plperl:
>>> * PL/Perl subroutines are now given names (Tim Bunce)
>>> This is for the use of profiling and code coverage tools. DIDN'T
>>> THEY HAVE NAMES BEFORE?
>>>
>>> If whoever put this in the release notes had bothered to ask I am sure
>>> we would have been happy to explain.
>>>
>> You don't need to complain, just fix it. The point of the comment is
>> that more explanation is needed.
>>
>
> I think you can just delete it. It's too esoteric to be worth noting.
>
> Tim.
>
> p.s. It also turned to be insufficiently useful for NYTProf since it
> doesn't also update some internals to record the 'filename' and line
> number range of the sub. So PostgreSQL::PLPerl::NYTProf works around
> that by wrapping mkfuncsrc() to edit the generated perl code to include
> a subname in the usual "sub $name { ... }" style. I may offer a patch
> for 9.1 to to make it work that way.
>
>
I put this aside to think about it.

If the "feature" is not any use should we rip it out? We pretty much
included it because you said it was what you needed for the profiler.

I'm seriously nervous about adding function names - making functions
directly callable like that could be a major footgun recipe, since now
we are free to hide some stuff in the calling mechanism, and can (and
have in the past) changed that to suit our needs, e.g. when we added
trigger support via a hidden function argument. That has been safe
precisely because nobody has had a handle on the subroutine which would
enable it to be called directly from perl code. There have been
suggestions in the past of using this for other features, so we should
not assume that the calling mechanism is fixed in stone.

Perhaps we could decorate the subs with attributes, or is that not
doable for anonymous subs?

cheers

andrew

On Sun, May 30, 2010 at 6:58 PM, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
> Tim Bunce wrote:
>> p.s. It also turned to be insufficiently useful for NYTProf since it
>> doesn't also update some internals to record the 'filename' and line
>> number range of the sub. So PostgreSQL::PLPerl::NYTProf works around
>> that by wrapping mkfuncsrc() to edit the generated perl code to include
>> a subname in the usual "sub $name { ... }" style. I may offer a patch
>> for 9.1 to to make it work that way.
>
> I put this aside to think about it.
>
> If the "feature" is not any use should we rip it out? We pretty much
> included it because you said it was what you needed for the profiler.
>
> I'm seriously nervous about adding function names - making functions
> directly callable like that could be a major footgun recipe, since now we
> are free to hide some stuff in the calling mechanism, and can (and have in
> the past) changed that to suit our needs, e.g. when we added trigger support
> via a hidden function argument. That has been safe precisely because nobody
> has had a handle on the subroutine which would enable it to be called
> directly from perl code. There have been suggestions in the past of using
> this for other features, so we should not assume that the calling mechanism
> is fixed in stone.
>
> Perhaps we could decorate the subs with attributes, or is that not doable
> for anonymous subs?

This is still on our open items list - should we do anything about it?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

[Sorry for the delay. I'd stopped checking the pgsql mailing lists.
Thanks to David Wheeler and Josh Berkus for the heads-up.]

On Sun, May 30, 2010 at 06:58:32PM -0400, Andrew Dunstan wrote:
>
> Tim Bunce wrote:
> >On Mon, May 17, 2010 at 11:34:37AM -0400, Tom Lane wrote:
> >>Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> >>>Why do the release notes say this, under plperl:
> >>> * PL/Perl subroutines are now given names (Tim Bunce)
> >>> This is for the use of profiling and code coverage tools. DIDN'T
> >>> THEY HAVE NAMES BEFORE?
> >>> If whoever put this in the release notes had bothered
> >>>to ask I am sure we would have been happy to explain.
> >>You don't need to complain, just fix it. The point of the comment is
> >>that more explanation is needed.
> >
> >I think you can just delete it. It's too esoteric to be worth noting.
> >
> >Tim.
> >
> >p.s. It also turned to be insufficiently useful for NYTProf since it
> >doesn't also update some internals to record the 'filename' and line
> >number range of the sub. So PostgreSQL::PLPerl::NYTProf works around
> >that by wrapping mkfuncsrc() to edit the generated perl code to include
> >a subname in the usual "sub $name { ... }" style. I may offer a patch
> >for 9.1 to to make it work that way.
>
> I put this aside to think about it.
>
> If the "feature" is not any use should we rip it out? We pretty much
> included it because you said it was what you needed for the
> profiler.

Yes, it can go.

*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
*************** plperl_create_sub(plperl_proc_desc *prod
*** 1319,1328 ****
(errmsg("didn't get a CODE ref from compiling %s",
prodesc->proname)));

- /* give the subroutine a proper name in the main:: symbol table */
- CvGV(SvRV(subref)) = (GV *) newSV(0);
- gv_init(CvGV(SvRV(subref)), PL_defstash, subname, strlen(subname), TRUE);
-
prodesc->reference = subref;

return;

> I'm seriously nervous about adding function names - making functions
> directly callable like that could be a major footgun recipe, since
> now we are free to hide some stuff in the calling mechanism, and can
> (and have in the past) changed that to suit our needs, e.g. when we
> added trigger support via a hidden function argument. That has been
> safe precisely because nobody has had a handle on the subroutine
> which would enable it to be called directly from perl code. There
> have been suggestions in the past of using this for other features,
> so we should not assume that the calling mechanism is fixed in stone.

Agreed. It is a very hard to use footgun though because the oid is
included in the name. It's certainly not something anyone would shoot
themselves with by accident.

[Speaking of calling conventions: I never did get time for some decent
performance optimization. I'd like to get rid of the explicit extra
trigger data argument and corresponding "local $_TD=shift;" prologue.
That could be done much faster in C.]

> Perhaps we could decorate the subs with attributes, or is that not
> doable for anonymous subs?

Perhaps. I'll look into it when I get around to working on the PL/Perl
NYTProf again. For the profiler it would be enough to only enable the
naming when the appropriate perl debugging flag bits are set, so it
wouldn't happen normally.

Tim.

Tim Bunce wrote:
>>
>> If the "feature" is not any use should we rip it out? We pretty much
>> included it because you said it was what you needed for the
>> profiler.
>>
>
> Yes, it can go.
>
>
>

Done.

cheers

andrew