Re: Add on_plperl_init and on_plperlu_init to plperl UPDATE 3 [PATCH]

This is the third update to the fourth of the patches to be split out
from the former 'plperl feature patch 1'.

Changes in this patch:

- Added plperl.on_plperl_init and plperl.on_plperlu_init GUCs
Both are PGC_SUSET
SPI functions are not available when the code is run.
Errors are detected and reported as ereport(ERROR, ...)
Corresponding documentation and tests for both.

- Renamed plperl.on_perl_init to plperl.on_init

- Improved state management of select_perl_context()
An error during interpreter initialization will leave
the state (interp_state etc) unchanged.

- The utf8fix code has been greatly simplified.

- More code comments re PGC_SUSET and no access to SPI functions.

Tim.

On Fri, Feb 5, 2010 at 06:40, Tim Bunce <Tim(dot)Bunce(at)pobox(dot)com> wrote:
> This is the third update to the fourth of the patches to be split out
> from the former 'plperl feature patch 1'.
>
> Changes in this patch:
>
> - Added plperl.on_plperl_init and plperl.on_plperlu_init GUCs
>    Both are PGC_SUSET
>    SPI functions are not available when the code is run.
>    Errors are detected and reported as ereport(ERROR, ...)
>    Corresponding documentation and tests for both.

*sniffle* OK I think I agree with everyone else on setting this as
PGC_SUSET until we can either prove it can be USERSET or we can fix it
so we can check permissions on SET properly. It seems if you really
wanted a user to be able to set it you should be able to define a
SECURITY DEFINER function that sets it to a string you pass in or
something. Obviously not part of core postgres...

> - Renamed plperl.on_perl_init to plperl.on_init

*shrug* OK (I think there was still some flack on this var, but I
think its ok-- we can discuss that in a separate thread if people
still disagree :) )

> - Improved state management of select_perl_context()
>    An error during interpreter initialization will leave
>    the state (interp_state etc) unchanged.
>
> - The utf8fix code has been greatly simplified.
>
> - More code comments re PGC_SUSET and no access to SPI functions.

I like the doc changes and think the new comment about %_SHARED being
unsafe is good.

All looks good to me. Ill mark it as "Ready for Committer"

Tim Bunce wrote:
> This is the third update to the fourth of the patches to be split out
> from the former 'plperl feature patch 1'.
>
> Changes in this patch:
>
> - Added plperl.on_plperl_init and plperl.on_plperlu_init GUCs
> Both are PGC_SUSET
> SPI functions are not available when the code is run.
> Errors are detected and reported as ereport(ERROR, ...)
> Corresponding documentation and tests for both.
>
> - Renamed plperl.on_perl_init to plperl.on_init
>
> - Improved state management of select_perl_context()
> An error during interpreter initialization will leave
> the state (interp_state etc) unchanged.
>
> - The utf8fix code has been greatly simplified.
>
> - More code comments re PGC_SUSET and no access to SPI functions.
>
>

The docs on this patch need some cleaning up and expanding:

* The possible insecurity of %_SHARED needs expanding.
* The docs still refer to plperl.on_untrusted_init
* plperl.on_plperl_init and plperl.on_plperlu_init can be documented
together rather than repeating the same stuff almost word for word.
* extra examples for these two, and an explanation of why one might
want to use each of the three on*init settings would be good.

I'll continue reviewing the patch, but these things at least need fixing.

cheers

andrew

On Sun, Feb 07, 2010 at 08:25:33PM -0500, Andrew Dunstan wrote:
> Tim Bunce wrote:
> >This is the third update to the fourth of the patches to be split out
> >from the former 'plperl feature patch 1'.
> >
> >Changes in this patch:
> >
> >- Added plperl.on_plperl_init and plperl.on_plperlu_init GUCs
> > Both are PGC_SUSET
> > SPI functions are not available when the code is run.
> > Errors are detected and reported as ereport(ERROR, ...)
> > Corresponding documentation and tests for both.
> >
> >- Renamed plperl.on_perl_init to plperl.on_init
> >
> >- Improved state management of select_perl_context()
> > An error during interpreter initialization will leave
> > the state (interp_state etc) unchanged.
> >
> >- The utf8fix code has been greatly simplified.
> >
> >- More code comments re PGC_SUSET and no access to SPI functions.
>
>
> The docs on this patch need some cleaning up and expanding:
>
> * The possible insecurity of %_SHARED needs expanding.

I tried. I couldn't decide how to expand what Tom Lane suggested
(http://archives.postgresql.org/message-id/1344.1265223887@sss.pgh.pa.us)
without it turning into a sprawling security tutorial.

So, since his suggestion seemed complete enough (albeit fairly vague),
I just used it almost verbatim.

Also, the PL/Tcl docs don't mention the issue at all and the PL/Python
docs say only "The global dictionary GD is public data, available to all
Python functions within a session. Use with care."

The wording in the PL/Python docs seems better ("available to all ...
use with care"), so I've adopted the same kind of wording.

> * The docs still refer to plperl.on_untrusted_init

Oops. Thanks. Fixed.

> * plperl.on_plperl_init and plperl.on_plperlu_init can be documented
> together rather than repeating the same stuff almost word for word.

Ok. Done.

> * extra examples for these two, and an explanation of why one might
> want to use each of the three on*init settings would be good.

plperl.on_init has an example that seems fairly self-explantory.
I've added one to the on_plperl_init/on_plperlu_init section that
also explains how a superuser can use ALTER USER ... SET .... to set
a value for a non-superuser.

> I'll continue reviewing the patch, but these things at least need fixing.

I've an updated patch ready to go. I'll hold on to it for now.
Let me know if you have any more issues, or not.
Thanks.

Tim.

On Mon, Feb 08, 2010 at 01:44:16PM +0000, Tim Bunce wrote:
>
> > I'll continue reviewing the patch, but these things at least need fixing.

Here's an updated patch. The only changes relative to the previous
version are in the docs, as I outlined in the previous message.

I'll add it to the commitfest.

Tim.