Miscellaneous changes to plperl [PATCH]

This is the second of the patches to be split out from the former
'plperl feature patch 1'.

Changes in this patch:

- Allow (ineffective) use of 'require' in plperl
If the required module is not already loaded then it dies.
So "use strict;" now works in plperl.

- Pre-load the feature module if perl >= 5.10.
So "use feature :5.10;" now works in plperl.

- Stored procedure subs are now given names.
The names are not visible in ordinary use, but they make
tools like Devel::NYTProf and Devel::Cover _much_ more useful.

- Simplified and generalized the subroutine creation code.
Now one code path for generating sub source code, not four.
Can generate multiple 'use' statements with specific imports
(which handles plperl.use_strict currently and can easily
be extended to handle a plperl.use_feature=':5.12' in future).

- Disallows use of Safe version 2.20 which is broken for PL/Perl.
http://rt.perl.org/rt3/Ticket/Display.html?id=72068

- Assorted minor optimizations by pre-growing data structures.

This patch will apply cleanly over the 'add functions' patch:
https://commitfest.postgresql.org/action/patch_view?id=264

Tim.

On Jan 14, 2010, at 8:07 AM, Tim Bunce wrote:

> - Stored procedure subs are now given names.
> The names are not visible in ordinary use, but they make
> tools like Devel::NYTProf and Devel::Cover _much_ more useful.

Wasn't this in the previous patch, too?

Best,

David

On Thu, Jan 14, 2010 at 09:34:42AM -0800, David E. Wheeler wrote:
> On Jan 14, 2010, at 8:07 AM, Tim Bunce wrote:
>
> > - Stored procedure subs are now given names.
> > The names are not visible in ordinary use, but they make
> > tools like Devel::NYTProf and Devel::Cover _much_ more useful.
>
> Wasn't this in the previous patch, too?

Ah, I see it was in the description of the previous patch but not in the
patch itself. Thanks. I'll add a note to the commitfest.

Tim.

On Thu, Jan 14, 2010 at 05:49:54PM +0000, Tim Bunce wrote:
> On Thu, Jan 14, 2010 at 09:34:42AM -0800, David E. Wheeler wrote:
> > On Jan 14, 2010, at 8:07 AM, Tim Bunce wrote:
> >
> > > - Stored procedure subs are now given names.
> > > The names are not visible in ordinary use, but they make
> > > tools like Devel::NYTProf and Devel::Cover _much_ more useful.
> >
> > Wasn't this in the previous patch, too?
>
> Ah, I see it was in the description of the previous patch but not in
> the patch itself. Thanks. I'll add a note to the commitfest.

A description here would help, too :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Thu, Jan 14, 2010 at 09:07, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> - Allow (ineffective) use of 'require' in plperl
>    If the required module is not already loaded then it dies.
>    So "use strict;" now works in plperl.

[ BTW I think this is awesome! ]

Id vote for use warnings; as well.

> - Stored procedure subs are now given names.
>    The names are not visible in ordinary use, but they make
>    tools like Devel::NYTProf and Devel::Cover _much_ more useful.

This needs to quote at least '. Any others you can think of? Also I
think we should sort the imports in ::mkfunsort so that they are
stable.

The cleanups were nice, the code worked as described. Other than the
quoting issue it looks good to me. Find below an incremental patch
that fixes the items above.

diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
index daef469..fa5df0a 100644
--- a/src/pl/plperl/plc_perlboot.pl
+++ b/src/pl/plperl/plc_perlboot.pl
@@ -27,16 +27,14 @@ sub ::mkfuncsrc {
my $BEGIN = join "\n", map {
my $names = $imports->{$_} || [];
"$_->import(qw(@$names));"
- } keys %$imports;
+ } sort keys %$imports;
$BEGIN &&= "BEGIN { $BEGIN }";

$name =~ s/\\/\\\\/g;
$name =~ s/::|'/_/g; # avoid package delimiters
+ $name =~ s/'/\'/g;

- my $funcsrc;
- $funcsrc .= qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN
$prolog $src } ];
- #warn "plperl mkfuncsrc: $funcsrc\n";
- return $funcsrc;
+ return qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog $src } ];
}

# see also mksafefunc() in plc_safe_ok.pl
diff --git a/src/pl/plperl/plc_safe_ok.pl b/src/pl/plperl/plc_safe_ok.pl
index 8d35357..79d64ce 100644
--- a/src/pl/plperl/plc_safe_ok.pl
+++ b/src/pl/plperl/plc_safe_ok.pl
@@ -25,6 +25,7 @@ $PLContainer->share(qw[&elog &return_next
$PLContainer->permit(qw[caller]);
::safe_eval(q{
require strict;
+ require warnings;
require feature if $] >= 5.010000;
1;
}) or die $@;

On Jan 22, 2010, at 7:59 PM, Alex Hunsaker wrote:

> $name =~ s/::|'/_/g; # avoid package delimiters
> + $name =~ s/'/\'/g;

Looks to me like ' is already handled in the line above the one you added, no?

David

On Sat, Jan 23, 2010 at 11:30, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> On Jan 22, 2010, at 7:59 PM, Alex Hunsaker wrote:
>
>>    $name =~ s/::|'/_/g; # avoid package delimiters
>> +   $name =~ s/'/\'/g;
>
> Looks to me like ' is already handled in the line above the one you added, no?

Well no, i suppose we could fix that via:
$name =~ s/[:|']/_/g;

Im betting that was the intent.

On Jan 23, 2010, at 11:20 AM, Alex Hunsaker wrote:

> Well no, i suppose we could fix that via:
> $name =~ s/[:|']/_/g;
>
> Im betting that was the intent.

Doubtful. In Perl, the package separator is either `::` or `'` (for hysterical reasons). So the original code was replacing any package separator with a single underscore. Your regex would change This::Module to This__Module, which I'm certain was not the intent.

Best,

David

On Sat, Jan 23, 2010 at 12:42, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> On Jan 23, 2010, at 11:20 AM, Alex Hunsaker wrote:
>
>> Well no, i suppose we could fix that via:
>> $name =~ s/[:|']/_/g;
>>
>> Im betting that was the intent.
>
> Doubtful. In Perl, the package separator is either `::` or `'` (for hysterical reasons). So the original code was replacing any package separator with a single underscore. Your regex would change This::Module to This__Module, which I'm certain was not the intent.

Haha, yep your right. I could have sworn I tested it with a function
name with a ' and it broke. But your obviously right :)

On Fri, Jan 22, 2010 at 08:59:10PM -0700, Alex Hunsaker wrote:
> On Thu, Jan 14, 2010 at 09:07, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> > - Allow (ineffective) use of 'require' in plperl
> >    If the required module is not already loaded then it dies.
> >    So "use strict;" now works in plperl.
>
> [ BTW I think this is awesome! ]

Thanks!

> I'd vote for use warnings; as well.

I would to, but sadly it's not that simple.

warnings uses Carp and Carp uses eval { ... } and, owing to a sad bug in
perl < 5.11.4, Safe can't distinguish between eval "..." and eval {...}
http://rt.perl.org/rt3/Ticket/Display.html?id=70970
So trying to load warnings fails (at least for some versions of perl).

I have a version of my final "Package namespace and Safe init cleanup
for plperl" that works around that. I opted to post a less potentially
controversial version of that patch in the end. If you think allowing
plperl code to 'use warnings;' is important (and I'd tend to agree)
then I'll update that final patch.

> > - Stored procedure subs are now given names.
> >    The names are not visible in ordinary use, but they make
> >    tools like Devel::NYTProf and Devel::Cover _much_ more useful.
>
> This needs to quote at least '. Any others you can think of? Also I
> think we should sort the imports in ::mkfunsort so that they are
> stable.

Sort for stability, yes. The quoting is fine though (I see you've come
to the same conclusion via David).

> The cleanups were nice, the code worked as described.

Thanks.

> Other than the quoting issue it looks good to me. Find below an
> incremental patch that fixes the items above.

> diff --git a/src/pl/plperl/plc_perlboot.pl b/src/pl/plperl/plc_perlboot.pl
> index daef469..fa5df0a 100644
> --- a/src/pl/plperl/plc_perlboot.pl
> +++ b/src/pl/plperl/plc_perlboot.pl
> @@ -27,16 +27,14 @@ sub ::mkfuncsrc {
> my $BEGIN = join "\n", map {
> my $names = $imports->{$_} || [];
> "$_->import(qw(@$names));"
> - } keys %$imports;
> + } sort keys %$imports;

Ok, good.

> $name =~ s/\\/\\\\/g;
> $name =~ s/::|'/_/g; # avoid package delimiters
> + $name =~ s/'/\'/g;

Not needed.

> - my $funcsrc;
> - $funcsrc .= qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog $src } ];
> - #warn "plperl mkfuncsrc: $funcsrc\n";
> - return $funcsrc;
> + return qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog $src } ];
> }

Ok. (I don't think that'll clash with any later patches.)

> # see also mksafefunc() in plc_safe_ok.pl
> diff --git a/src/pl/plperl/plc_safe_ok.pl b/src/pl/plperl/plc_safe_ok.pl
> index 8d35357..79d64ce 100644
> --- a/src/pl/plperl/plc_safe_ok.pl
> +++ b/src/pl/plperl/plc_safe_ok.pl
> @@ -25,6 +25,7 @@ $PLContainer->share(qw[&elog &return_next
> $PLContainer->permit(qw[caller]);
> ::safe_eval(q{
> require strict;
> + require warnings;
> require feature if $] >= 5.010000;
> 1;
> }) or die $@;

Not viable, sadly.

> On Sat, Jan 23, 2010 at 12:42, David E. Wheeler <david(at)kineticode(dot)com> wrote:
> > On Jan 23, 2010, at 11:20 AM, Alex Hunsaker wrote:
> >
> >> Well no, i suppose we could fix that via:
> >> $name =~ s/[:|']/_/g;
> >>
> >> Im betting that was the intent.
> >
> > Doubtful. In Perl, the package separator is either `::` or `'` (for hysterical reasons). So the original code was replacing any package separator with a single underscore. Your regex would change This::Module to This__Module, which I'm certain was not the intent.
>
> Haha, yep your right. I could have sworn I tested it with a function
> name with a ' and it broke. But your obviously right :)

I could have sworn I wrote a test file with a bunch of stressful names.
All seems well though:

template1=# create or replace function "a'b*c}d!"() returns text language plperl as '42'; CREATE FUNCTION
template1=# select "a'b*c}d!"();
a'b*c}d!
----------
42

So, what now? Should I resend the patch with the two 'ok' changes above
included, or can the committer make those very minor changes?

Tim.

Tim Bunce wrote:
>> - } keys %$imports;
>> + } sort keys %$imports;
>>
>
> Ok, good.
>
>
>> - my $funcsrc;
>> - $funcsrc .= qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog $src } ];
>> - #warn "plperl mkfuncsrc: $funcsrc\n";
>> - return $funcsrc;
>> + return qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog $src } ];
>>
>>
>
> Ok. (I don't think that'll clash with any later patches.)
>
>
> So, what now? Should I resend the patch with the two 'ok' changes above
> included, or can the committer make those very minor changes?
>
>
>

I'll pick these up, if Alex marks it ready for committer.

cheers

andrew

On Sat, Jan 23, 2010 at 16:26, Andrew Dunstan <andrew(at)dunslane(dot)net> wrote:
>
>
> Tim Bunce wrote:
>>>
>>> -   } keys %$imports;
>>> +   } sort keys %$imports;
>>>
>>
>> Ok, good.
>>
>>
>>>
>>> -   my $funcsrc;
>>> -   $funcsrc .= qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog
>>> $src } ];
>>> -   #warn "plperl mkfuncsrc: $funcsrc\n";
>>> -   return $funcsrc;
>>> +   return qq[ undef *{'$name'}; *{'$name'} = sub { $BEGIN $prolog $src }
>>> ];
>>>
>>>
>>
>> Ok. (I don't think that'll clash with any later patches.)
>>
>>
>> So, what now? Should I resend the patch with the two 'ok' changes above
>> included, or can the committer make those very minor changes?
>>
>>
>>
>
> I'll pick these up, if Alex marks it ready for committer.

Done.

On Sat, Jan 23, 2010 at 16:16, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> On Fri, Jan 22, 2010 at 08:59:10PM -0700, Alex Hunsaker wrote:
>> On Thu, Jan 14, 2010 at 09:07, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
>> I'd vote for use warnings; as well.
>
> I would to, but sadly it's not that simple.
>
> warnings uses Carp and Carp uses eval { ... } and, owing to a sad bug in
> perl < 5.11.4, Safe can't distinguish between eval "..." and eval {...}
> http://rt.perl.org/rt3/Ticket/Display.html?id=70970
> So trying to load warnings fails (at least for some versions of perl).

Well that stinks.

> I have a version of my final "Package namespace and Safe init cleanup
> for plperl" that works around that. I opted to post a less potentially
> controversial version of that patch in the end. If you think allowing
> plperl code to 'use warnings;' is important (and I'd tend to agree)
> then I'll update that final patch.

Sounds good.

On Sat, Jan 23, 2010 at 06:40:03PM -0700, Alex Hunsaker wrote:
> On Sat, Jan 23, 2010 at 16:16, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> > On Fri, Jan 22, 2010 at 08:59:10PM -0700, Alex Hunsaker wrote:
> >> On Thu, Jan 14, 2010 at 09:07, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> >> I'd vote for use warnings; as well.
> >
> > I would to, but sadly it's not that simple.
> >
> > warnings uses Carp and Carp uses eval { ... } and, owing to a sad bug in
> > perl < 5.11.4, Safe can't distinguish between eval "..." and eval {...}
> > http://rt.perl.org/rt3/Ticket/Display.html?id=70970
> > So trying to load warnings fails (at least for some versions of perl).
>
> Well that stinks.

Yeap. I was amazed that no one had run into it before.

> > I have a version of my final "Package namespace and Safe init cleanup
> > for plperl" that works around that. I opted to post a less potentially
> > controversial version of that patch in the end. If you think allowing
> > plperl code to 'use warnings;' is important (and I'd tend to agree)
> > then I'll update that final patch.
>
> Sounds good.

FYI I've an updated patch ready but I'll wait till the commitfest has
got 'closer' as there's a fair chance a further update will be needed
anyway to make a patch that applies cleanly.

Tim.

Tim Bunce wrote:
>
> FYI I've an updated patch ready but I'll wait till the commitfest has
> got 'closer' as there's a fair chance a further update will be needed
> anyway to make a patch that applies cleanly.
>
>
>

I want to deal with this today or tomorrow, so don't sit on it, please.

cheers

andrew

On Mon, Jan 25, 2010 at 11:09:12AM -0500, Andrew Dunstan wrote:
>
> Tim Bunce wrote:
> >
> >FYI I've an updated patch ready but I'll wait till the commitfest has
> >got 'closer' as there's a fair chance a further update will be needed
> >anyway to make a patch that applies cleanly.
>
> I want to deal with this today or tomorrow, so don't sit on it, please.

Okay. I'll post it as a reply to the original and add it to the commitfest.

Tim.