Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-25 Thread Tim Bunce 
On Sat, Jan 23, 2010 at 06:40:03PM -0700, Alex Hunsaker wrote:
 On Sat, Jan 23, 2010 at 16:16, Tim Bunce tim.bu...@pobox.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.bu...@pobox.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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-25 Thread Andrew Dunstan 



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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-25 Thread Tim Bunce 
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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread David E. Wheeler 
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
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread Alex Hunsaker 
On Sat, Jan 23, 2010 at 11:30, David E. Wheeler da...@kineticode.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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread David E. Wheeler 
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
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread Alex Hunsaker 
On Sat, Jan 23, 2010 at 12:42, David E. Wheeler da...@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 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 :)

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread Tim Bunce 
On Fri, Jan 22, 2010 at 08:59:10PM -0700, Alex Hunsaker wrote:
 On Thu, Jan 14, 2010 at 09:07, Tim Bunce tim.bu...@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.01;
 1;
  }) or die $@;

Not viable, sadly.


 On Sat, Jan 23, 2010 at 12:42, David E. Wheeler da...@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 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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread Andrew Dunstan 



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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread Alex Hunsaker 
On Sat, Jan 23, 2010 at 16:26, Andrew Dunstan and...@dunslane.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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-23 Thread Alex Hunsaker 
On Sat, Jan 23, 2010 at 16:16, Tim Bunce tim.bu...@pobox.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.bu...@pobox.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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-22 Thread Alex Hunsaker 
On Thu, Jan 14, 2010 at 09:07, Tim Bunce tim.bu...@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! ]

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.01;
1;
 }) or die $@;


plperl_misc_inc_ah.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-14 Thread David E. Wheeler 
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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-14 Thread Tim Bunce 
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.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-14 Thread David Fetter 
On Thu, Jan 14, 2010 at 05:49:54PM +, 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 da...@fetter.org http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers