Thread

1. Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Tim Bunce <tim.bunce@pobox.com> — 2010-01-30T23:16:08Z

   This is an update to the final plperl patch in the series from me.
   
   Changes in the original patch:
   
   - Moved internal functions out of main:: namespace
       into PostgreSQL::InServer and PostgreSQL::InServer::safe
   
   - Restructured Safe compartment setup code
       to generalize and separate the data from the logic.
   
   Neither change has any user visible effects.
   
   Additional changes in the second version:
   
   - Further generalized the 'what to load into Safe compartment' logic.
   
   - Added the 'warnings' pragma to the list of modules to load into Safe.
     So plperl functions can now "use warnings;" - added test for that.
   
   - Added 'use 5.008001;' to plc_perlboot.pl as a run-time check to
     complement the configure-time check added by Tom Lane recently.
   
   Additional changes in this version:
   
   - Rebased over recent HEAD plus "on_trusted_init" patch
   
   - Made plc_safe_ok.pl code idempotent to avoid risk of problems
     from repeated initialization attempts e.g. if on_trusted_init code
     throws an exception so initialization doesn't complete.
   
   - Fixed 'require strict' to enable 'caller' opcode
     (needed for Perl >=5.10)
   
   - Ensure Safe container opmask is restored even if @EvalInSafe code
     throws an exception.
   
   - Changed errmsg("didn't get a GLOB ...") to use errmsg_internal().
   
   Tim.
   

2. Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Alex Hunsaker <badalex@gmail.com> — 2010-01-31T01:38:59Z

   On Sat, Jan 30, 2010 at 16:16, Tim Bunce <Tim.Bunce@pobox.com> wrote:
   > This is an update to the final plperl patch in the series from me.
   >
   > Changes in the original patch:
   
   plc_safe_ok.pl seems to loose its CVS $PostgreSQL$ keyword.
   
   > - Ensure Safe container opmask is restored even if @EvalInSafe code
   >  throws an exception.
   
   Maybe we could be a bit smarter about this and avoid the problem? Im
   thinking either (for @ShareIntoSafe as well):
   
   1) always reinit @EvalInSafe at the top of plc_safe_ok.pl
   our @EvalInSafe = [ ]; ....
   
   Seems like a non-starter, why have the 'our' at all?
   
   2)Change EvalInSafe to be a hash so at least the problem with
   duplicates goes away:
   $EvalInSafe{'strict'} = 'caller';
   
   Then again maybe its fine the way it is.  Thoughts?
   
   This makes me think maybe we should not expose it at all.  Its not
   like you can tell people oh you have something you need in your plperl
   safe?  Just stick it in @PostgreSQL::InServer::safe::EvalInSafe.  As
   we might change this *undocumented* interface in the future.
   
   That being said *im* ok with it.  Its useful from a debug standpoint.
   
   Thoughts?
   
   

3. Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Tim Bunce <tim.bunce@pobox.com> — 2010-02-01T10:58:46Z

   On Sat, Jan 30, 2010 at 06:38:59PM -0700, Alex Hunsaker wrote:
   > On Sat, Jan 30, 2010 at 16:16, Tim Bunce <Tim.Bunce@pobox.com> wrote:
   > > This is an update to the final plperl patch in the series from me.
   > >
   > > Changes in the original patch:
   > 
   > plc_safe_ok.pl seems to loose its CVS $PostgreSQL$ keyword.
   
   Probably a slip-up when I merged the changes from HEAD up through my
   chain of branches.
   
   > > - Ensure Safe container opmask is restored even if @EvalInSafe code
   > >  throws an exception.
   > 
   > Maybe we could be a bit smarter about this and avoid the problem?
   > Im thinking either (for @ShareIntoSafe as well):
   > 
   > 1) always reinit @EvalInSafe at the top of plc_safe_ok.pl
   > our @EvalInSafe = [ ]; ....
   > 
   > Seems like a non-starter, why have the 'our' at all?
   
   Yeap.
   
   > 2)Change EvalInSafe to be a hash so at least the problem with
   > duplicates goes away:
   > $EvalInSafe{'strict'} = 'caller';
   > 
   > Then again maybe its fine the way it is.  Thoughts?
   
   A better approach would be for the plperl.c code to keep track of
   initialization with a finer granularity.  Specifically track the fact
   that plc_safe_ok.pl ran ok so not re-run it if on_trusted_init fails.
   But that would be a more invasive change for no significant gain so
   didn't seem appropriate at this point.
   
   The current code works fine, and behaves well in failure modes, so I
   think it's okay the way it is.
   
   I hope to work on plperl.c some more for the 9.1 release (if my
   employer's generosity continues). Mainly to switch to using
   PERL_NO_GET_CONTEXT to simplify state management and improve
   performance (getting rid of the many many hidden calls to
   pthread_getspecific). That would be a good time to rework this area.
   
   > This makes me think maybe we should not expose it at all.  Its not
   > like you can tell people oh you have something you need in your plperl
   > safe?  Just stick it in @PostgreSQL::InServer::safe::EvalInSafe.  As
   > we might change this *undocumented* interface in the future.
   > 
   > That being said *im* ok with it.  Its useful from a debug standpoint.
   
   Yes. And, as I mentioned previously, I expect people like myself, David
   Wheeler, and others to experiment with the undocumented functionality
   and define and document a good API to it for the 9.1 release.
   
   I'd much rather get this change in than shoot for a larger change that
   doesn't get committed due to long-running discussions.  (Which seems
   more likely as Andrew's going to be less available for the rest of the
   commitfest.)
   
   Tim.
   
   p.s. If there is interest in defining a documented API (for DBAs to
   control what gets loaded into Safe and shared with it) for *9.0*
   then that could be worked on, once this pach is in, ready for the
   next commitfest.
   
   

4. Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Robert Haas <robertmhaas@gmail.com> — 2010-02-01T15:46:10Z

   On Mon, Feb 1, 2010 at 5:58 AM, Tim Bunce <Tim.Bunce@pobox.com> wrote:
   > p.s. If there is interest in defining a documented API (for DBAs to
   > control what gets loaded into Safe and shared with it) for *9.0*
   > then that could be worked on, once this pach is in, ready for the
   > next commitfest.
   
   This is the last CommitFest for 9.0.  It's time to wind down
   development on this release and work on trying to get the release
   stabilized and out the door.
   
   This isn't intended as a disparagement of the work you're doing; I've
   thought about using PL/perl in the past and decided against it exactly
   because of some of the issues you're now fixing.  But we're really out
   of time to get things done for 9.0.
   
   ...Robert
   
   

5. Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Tim Bunce <tim.bunce@pobox.com> — 2010-02-01T17:01:06Z

   On Mon, Feb 01, 2010 at 10:46:10AM -0500, Robert Haas wrote:
   > On Mon, Feb 1, 2010 at 5:58 AM, Tim Bunce <Tim.Bunce@pobox.com> wrote:
   > > p.s. If there is interest in defining a documented API (for DBAs to
   > > control what gets loaded into Safe and shared with it) for *9.0*
   > > then that could be worked on, once this pach is in, ready for the
   > > next commitfest.
   > 
   > This is the last CommitFest for 9.0.  It's time to wind down
   > development on this release and work on trying to get the release
   > stabilized and out the door.
   > 
   > This isn't intended as a disparagement of the work you're doing; I've
   > thought about using PL/perl in the past and decided against it exactly
   > because of some of the issues you're now fixing.  But we're really out
   > of time to get things done for 9.0.
   
   Understood Robert. No problem. (You can't blame me for trying ;-)
   
   Tim.
   
   

6. Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Alex Hunsaker <badalex@gmail.com> — 2010-02-02T02:53:05Z

   On Mon, Feb 1, 2010 at 03:58, Tim Bunce <Tim.Bunce@pobox.com> wrote:
   > On Sat, Jan 30, 2010 at 06:38:59PM -0700, Alex Hunsaker wrote:
   >
   >> plc_safe_ok.pl seems to loose its CVS $PostgreSQL$ keyword.
   >Probably a slip-up when I merged the changes from HEAD up through my
   >chain of branches.
   
   Can you send an updated patch?  I think Andrew will probably fix it up
   anyway but better safe than sorry.
   
   >> That being said *im* ok with it.  Its useful from a debug standpoint.
   >
   > Yes. And, as I mentioned previously, I expect people like myself, David
   > Wheeler, and others to experiment with the undocumented functionality
   > and define and document a good API to it for the 9.1 release.
   
   Huh, I missed that.
   
   > I'd much rather get this change in than shoot for a larger change that
   > doesn't get committed due to long-running discussions.  (Which seems
   > more likely as Andrew's going to be less available for the rest of the
   > commitfest.)
   
   Plus its hard to get people to agree on anything GUCy (my new favorite
   pun) thats not well thought out and tested.
   
   Anyway yes I agree, but I thought I should at least raise it for
   discussion. You'll notice the patch has been marked "Ready for
   Commiter" this whole time. =)
   
   

7. Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]

   Tim Bunce <tim.bunce@pobox.com> — 2010-02-02T16:22:11Z

   On Mon, Feb 01, 2010 at 07:53:05PM -0700, Alex Hunsaker wrote:
   > On Mon, Feb 1, 2010 at 03:58, Tim Bunce <Tim.Bunce@pobox.com> wrote:
   > > On Sat, Jan 30, 2010 at 06:38:59PM -0700, Alex Hunsaker wrote:
   > >
   > >> plc_safe_ok.pl seems to loose its CVS $PostgreSQL$ keyword.
   > >Probably a slip-up when I merged the changes from HEAD up through my
   > >chain of branches.
   > 
   > Can you send an updated patch?  I think Andrew will probably fix it up
   > anyway but better safe than sorry.
   
   Attached. I'll add it to the commitfest.
   
   > Anyway yes I agree, but I thought I should at least raise it for
   > discussion. You'll notice the patch has been marked "Ready for
   > Commiter" this whole time. =)
   
   Thanks.
   
   Tim.