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

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.

On Sat, Jan 30, 2010 at 16:16, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)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?

On Sat, Jan 30, 2010 at 06:38:59PM -0700, Alex Hunsaker wrote:
> On Sat, Jan 30, 2010 at 16:16, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)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.

On Mon, Feb 1, 2010 at 5:58 AM, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)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

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(dot)Bunce(at)pobox(dot)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.

On Mon, Feb 1, 2010 at 03:58, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)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. =)

On Mon, Feb 01, 2010 at 07:53:05PM -0700, Alex Hunsaker wrote:
> On Mon, Feb 1, 2010 at 03:58, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)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.