Feature patch 1 for plperl - open issues

Thanks for all the feedback!

I'm going to try to summarize and address the issues raised.

(Meanwhile I've started spliting the patch into at least three parts,
per Andrew's suggestion: utility functions, the GUCs, the rest.)

* Long GUC values

I think the underlying assumption that they'll be long values isn't valid.
Any non-trivial use should be implemented by loading a private module, like
Andrew's example: 'use lib "/my/app"; use MyApp::Pg;' The lack of multi-line
GUCs will naturally encourage that and I'll ensure the docs do as well.

* Running arbitrary user-defined code in the postmaster

I think there's agreement that the ability to load code in the
postmaster is very useful (http://markmail.org/message/wl7dmcbcrkwv2jz2)
The concern is only over the safety of combining on_perl_init with
shared_preload_libraries.

Others have pointed out that the very purpose of shared_preload_libraries is to
load and run (init) user-defined code in the postmaster. Albeit in the form of
C code. I thought I'd sufficiently addressed the concerns that were raised
previously (http://markmail.org/message/u7zhkjkwmztk5hkw).

Specifically, on_perl_init is PGC_SUSET (I'll change this to PGC_SIGHUP
per Tom's recent email) and SPI functions aren't available for on_*_init code.
So the on_*_init code has no natural way to interact with the server.
If the code fails (dies/throws an exception) an error is raised.

* Running arbitrary user-defined code at backend shutdown

In the same way that on_*_init code has no natural way to interact with the
server I'm keen for the same to apply to END block code run at backend shutdown.
I'd assumed that the SPI functions would fail anyway during shutdown but I
hadn't explicitly tested for it (viewing it as a foot shooting gun).
I'll explicitly arrange for SPI functions to fail during server shutdown
and test for that in the next revision of the patch.

> What happens when the supplied code has errors takes an unreasonable amount
> of time to run, does something unsafe, depends on the backend not being in an
> error state already, etc etc?

Since there's no interaction with the server I think the only one of those that
needs addressing is "takes an unreasonable amount of time to run" and
(from a different reply) "delaying or even preventing shutdown".

I'm puzzled by that one as I don't see how it's different to the user doing
DO $$ sleep 99999 $$ language plperl; # or any other PL
I'd be grateful if someone could explain the difference.

* Visibility of shared library loading event

> loading of the plperl shared
> library should not be a semantically significant event from the user's
> viewpoint. If these GUCs were PGC_POSTMASTER it wouldn't be so bad, but
> Tim still seems to want them to be settable inside an individual session
> before the library is loaded, which makes the loading extremely visible.

The perl interpreter has an initial state. The only effect of on_*_init
should be to allow the user to choose a different initial state
(by running arbitrary code to move from the default state to another).

I presume some might say that the first use of a temporary table
should not be a semantically significant event from the user's
viewpoint. Yet the temp_buffers GUC says:

The setting can be changed within individual sessions, but only
up until the first use of temporary tables within a session

A practical use-case for this plperl.on_*trusted_init behaviour is
plperl.on_untrusted_init='use Devel::NYTProf::PgPLPerl'
(Or plperl.on_trusted_init if the DBA has pre-loaded it to make it available.)
NYTProf needs to be loaded before the code it's going to profile in order to
provide full functionality.

> As an example, if people were using such functionality then the DBA
> couldn't start preloading plperl for performance without breaking
> behavior that some of his users might be depending on.

This isn't the case. Users can only set the on_trusted_init and
on_untrusted_init GUCs. The code in those is only executed when the interpreter
is transitioned from its initial HELD state by the user's first use.

So the timing of plperl.on_*trusted_init execution isn't altered by
shared_preload_libraries. (The on_perl_init GUC, under DBA control,
is the only one executed at the time plperl is loaded.)

I'd be happy to document plperl.on_trusted_init and on_untrusted_init
in the Developer Options section of the docs if that would help
set user-expectations.

On Sun, Jan 10, 2010 at 07:26:01PM -0500, Tom Lane wrote:
>
> Just looking over this patch, I don't think it's nearly robust enough
> against initialization failures. The original code wasn't very good
> about that either, but that was (more or less) okay because it was
> executing predetermined, pretested code that we really don't expect to
> fail. I think the standard has to be a *lot* higher if we are going to
> execute user-supplied perl code there. You need to make sure that
> things are left in a reasonably consistent, safe state if an error
> occurs.

An exception from plperl.on_perl_init causes elog(ERROR, ...);
Currently an exception from plperl.on_trusted_init and on_untrusted_init
cause a WARNING. I'll change those to ERROR. If there are other issues in this
regard I'd appreciate specific details.

> Along the same line, there needs to be more effort put into the errors
> that can be thrown when one of these failures happen. The current
> messages don't follow our style guidelines very well, and aren't exposed
> for translation.

Per http://developer.postgresql.org/pgdocs/postgres/error-message-reporting.html
and http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html
Okay, I'll fix those.

Tim.