Re: Miscellaneous changes to plperl [PATCH]

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.