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

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.