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

[HACKERS] Miscellaneous changes to plperl [PATCH]

2010-01-14 Thread Tim Bunce 
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.
diff --git a/doc/src/sgml/plperl.sgml b/doc/src/sgml/plperl.sgml
index 94db722..6fee031 100644
*** a/doc/src/sgml/plperl.sgml
--- b/doc/src/sgml/plperl.sgml
*** SELECT * FROM perl_set();
*** 285,313 
/para
  
para
!If you wish to use the literalstrict/ pragma with your code,
!the easiest way to do so is to commandSET/
!literalplperl.use_strict/literal to true.  This parameter affects
!subsequent compilations of applicationPL/Perl/ functions, but not
!functions already compiled in the current session.  To set the
!parameter before applicationPL/Perl/ has been loaded, it is
!necessary to have added quoteliteralplperl// to the xref
!linkend=guc-custom-variable-classes list in
!filenamepostgresql.conf/filename.
/para
  
para
!Another way to use the literalstrict/ pragma is to put:
  programlisting
  use strict;
  /programlisting
!in the function body.  But this only works in applicationPL/PerlU/
!functions, since the literaluse/ triggers a literalrequire/
!which is not a trusted operation.  In
!applicationPL/Perl/ functions you can instead do:
! programlisting
! BEGIN { strict-import(); }
! /programlisting
/para
   /sect1
  
--- 285,323 
/para
  
para
!If you wish to use the literalstrict/ pragma with your code you have a few options.
!For temporary global use you can commandSET/ literalplperl.use_strict/literal
!to true (see xref linkend=plperl.use_strict).
!This will affect subsequent compilations of applicationPL/Perl/
!functions, but not functions already compiled in the current session.
!For permanent global use you can set literalplperl.use_strict/literal
!to true in the filenamepostgresql.conf/filename file.
/para
  
para
!For permanent use in specific functions you can simply put:
  programlisting
  use strict;
  /programlisting
!at the top of the function body.
!   /para
! 
!   para
!   The literalfeature/ pragma is also available to functionuse/ if your Perl is version 5.10.0 or higher.
!   /para
! 
!  /sect1
! 
!  sect1 id=plperl-data
!   titleData Values in PL/Perl/title
! 
!   para
!The argument values supplied to a PL/Perl function's code are
!simply the input arguments converted to text form (just as if they
!had been displayed by a commandSELECT/command statement).
!Conversely, the functionreturn/function and functionreturn_next/function
!commands will accept any string that is acceptable input format
!for the function's declared return type.
/para
   /sect1
  
*** SELECT done();
*** 682,699 
   /sect2
   /sect1
  
-  sect1 id=plperl-data
-   titleData Values in PL/Perl/title
- 
-   para
-The argument values supplied to a PL/Perl function's code are
-simply the input arguments converted to text form (just as if they
-had been displayed by a commandSELECT/command statement).
-Conversely, the literalreturn/ command will accept any string
-that is acceptable input format for the function's declared return
-type.  So, within the PL/Perl function,
-all values are just text strings.
-   /para
   /sect1
  
   sect1 id=plperl-global
--- 692,697 
*** CREATE TRIGGER test_valid_id_trig
*** 1042,1049 
 itemizedlist
  listitem
   para
!   PL/Perl functions cannot call each other directly (because they
!   are anonymous subroutines inside Perl).
   /para
  /listitem
  
--- 1040,1046 
 itemizedlist
  listitem
   para
!   PL/Perl functions cannot call each other directly.
   /para
  /listitem
  
*** CREATE TRIGGER test_valid_id_trig
*** 1072,1077 
--- 1069,1076 
  /listitem
 /itemizedlist
/para
+  /sect2
+ 
   /sect1
  
  /chapter
diff

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