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.Bunce@pobox.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@kineticode.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
replacingany package separator with a single underscore. Your regex would change This::Module to This__Module, which
I'mcertain 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.