Skip site navigation (1) Skip section navigation (2)

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

From: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>
To: Alex Hunsaker <badalex(at)gmail(dot)com>
Cc: Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Package namespace and Safe init cleanup for plperl UPDATE 3 [PATCH]
Date: 2010-02-01 10:58:46
Message-ID: 20100201105846.GJ1141@timac.local (view raw or flat)
Thread:
Lists: pgsql-hackers
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.

In response to

Responses

pgsql-hackers by date

Next:From: Leonardo FDate: 2010-02-01 11:09:14
Subject: Re: About "Our CLUSTER implementation is pessimal" patch
Previous:From: Simon RiggsDate: 2010-02-01 10:46:06
Subject: Re: [COMMITTERS] pgsql: Write a WAL record whenever we perform an operation without

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group