Add on_perl_init and proper destruction to plperl [PATCH]

Tim Bunce wrote:

- Added plperl.on_perl_init GUC for DBA use (PGC_SIGHUP)
SPI functions are not available when the code is run.

- Added normal interpreter destruction behaviour
END blocks, if any, are run then objects are
destroyed, calling their DESTROY methods, if any.
SPI functions will die if called at this time.

OK, we've made good progress with the PL/Perl patches, and this one is
next on the queue.

It should also be noted that as proposed END blocks will not run at all
in the postmaster, even if perl is preloaded in the postmaster and the
preloaded code sets END handlers. That makes setting them rather safer,
ISTM.

So, are there still objections to applying this patch?

(Note, this is different from the proposal to specify on_trusted_init
and on_untrusted_init handlers. The on_perl_init handler would be run on
library load, and is mainly for the purpose of preloading perl modules
and the like).

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tim Bunce wrote:

- Added plperl.on_perl_init GUC for DBA use (PGC_SIGHUP)
SPI functions are not available when the code is run.

- Added normal interpreter destruction behaviour
END blocks, if any, are run then objects are
destroyed, calling their DESTROY methods, if any.
SPI functions will die if called at this time.

So, are there still objections to applying this patch?

Yes.

regards, tom lane

On Tue, Jan 26, 2010 at 23:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tim Bunce wrote:

- Added plperl.on_perl_init GUC for DBA use (PGC_SIGHUP)
SPI functions are not available when the code is run.

- Added normal interpreter destruction behaviour
END blocks, if any, are run then objects are
destroyed, calling their DESTROY methods, if any.
SPI functions will die if called at this time.

So, are there still objections to applying this patch?

Yes.

FWIW the atexit scares me to. I was thinking a good workaround
perhaps would be to provide a function that destroys the interpreter
(so that the END blocks get called). Tim would that work OK ? If we
are still worried about that hanging we can probably do something
hacky with alarm() and/or signals...

Maybe a good solid use case will help figure this out? Im assuming
the current one is to profile plperl functions and dump a prof file in
/tmp/ or some such (which happens at END time). Or did I miss the use
case in one of the other threads?

On Wed, Jan 27, 2010 at 12:46:42AM -0700, Alex Hunsaker wrote:

On Tue, Jan 26, 2010 at 23:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tim Bunce wrote:

- Added plperl.on_perl_init GUC for DBA use (PGC_SIGHUP)
SPI functions are not available when the code is run.

- Added normal interpreter destruction behaviour
END blocks, if any, are run then objects are
destroyed, calling their DESTROY methods, if any.
SPI functions will die if called at this time.

So, are there still objections to applying this patch?

Yes.

FWIW the atexit scares me to.

In what way, specifically?

I understand concerns about interacting with the database, so the
patch ensures that any use of spi functions throws an exception.

I don't recall any other concrete concerns.

Specifically, how is code that starts executing at the end of a session
different in risk to code that starts executing before the end of a session?

DO $$ while (1) { } $$ language plperl;

Tim.

Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tim Bunce wrote:

- Added plperl.on_perl_init GUC for DBA use (PGC_SIGHUP)
SPI functions are not available when the code is run.

- Added normal interpreter destruction behaviour
END blocks, if any, are run then objects are
destroyed, calling their DESTROY methods, if any.
SPI functions will die if called at this time.

So, are there still objections to applying this patch?

Yes.

I see I asked the wrong question. Start again.

What more should be done to make all or some of it acceptable?

cheers

andrew

On Wed, Jan 27, 2010 at 01:14:16AM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tim Bunce wrote:

- Added plperl.on_perl_init GUC for DBA use (PGC_SIGHUP)
SPI functions are not available when the code is run.

- Added normal interpreter destruction behaviour
END blocks, if any, are run then objects are
destroyed, calling their DESTROY methods, if any.
SPI functions will die if called at this time.

So, are there still objections to applying this patch?

Yes.

To focus the discussion I've looked back through all the messages from
you that relate to this issue so I can summarize and try to address your
objections.

Some I've split or presented out of order, most relate to earlier (less
restricted) versions of the patch before it was split out, and naturally
they are lacking some context, so I've included archive URLs.

Please forgive and correct me if I misrepresent you or your intent here.

Regarding the utility of plperl.on_perl_init and END:

http://archives.postgresql.org/message-id/18338.1260033447@sss.pgh.pa.us
The question is not about whether we think it's useful; the question
is about whether it's safe.

I agree.

Regarding visibility of changes to plperl.on_perl_init:

http://archives.postgresql.org/message-id/28618.1259952660@sss.pgh.pa.us
What is to happen if the admin changes the value when the system is already up?

If a GUC could be defined as PGC_BACKEND and only settable by superuser,
perhaps that would be a good fit. [GucContext seems to conflate some things.]
Meanwhile the _init name is meant to convey the fact that it's a
before-first-use GUC, like temp_buffers.

I'm happy to accept whatever you'd recommend by way of PGC_* GUC selection.
Documentation can note any caveats associated with combining
plperl.on_perl_init with shared_preload_libraries.

http://archives.postgresql.org/message-id/4516.1263168347@sss.pgh.pa.us
However, I think PGC_SIGHUP would be enough to address my basic
worry, which is that people shouldn't be depending on the ability to set
these things within an individual session.

The patch uses PGC_SIGHUP for plperl.on_perl_init.

http://archives.postgresql.org/message-id/8950.1259994082@sss.pgh.pa.us

Tom, what's your objection to Shlib load time being user-visible?

It's not really designed to be user-visible. Let me give you just
two examples:

* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. Later the transaction fails and is rolled
back. If loading plperl.so caused some user-visible things to happen,
should those be rolled back? If so, how do we get perl to play along?
If not, how do we get postgres to play along?

I believe that's addressed by spi functions being disabled when init code runs.

* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. This happens in the context of a superuser
calling a non-superuser security definer function, or perhaps vice
versa. Whose permissions apply to whatever the on_load code tries
to do? (Hint: every answer is wrong.)

I think that related to on_*trusted_init not plperl.on_perl_init, and
is also addressed by spi functions being disabled when init code runs.

That doesn't even begin to cover the problems with allowing any of
this to happen inside the postmaster. Recall that the postmaster
does not have any database access.

I believe that's addressed by spi functions being disabled when init code runs.

Furthermore, it is a very long
established reliability principle around here that the postmaster
process should do as little as possible, because every thing that it
does creates another opportunity to have a nonrecoverable failure.
The postmaster can recover if a child crashes, but the other way
round, not so much.

I understand that concern. Ultimately, though, that comes down to the
judgement of DBAs and the trust placed in them. They can already
load arbitrary code via shared_preload_libraries.

http://archives.postgresql.org/message-id/18338.1260033447@sss.pgh.pa.us

I think if we do this the on_perl_init setting should probably be
PGC_POSTMASTER, which would remove any issue about it changing
underneath us.

Yes, if the main intended usage is in combination with preloading perl
at postmaster start, it would be pointless to imagine that PGC_SIGHUP
is useful anyway.

http://archives.postgresql.org/message-id/17793.1260031296@sss.pgh.pa.us
Yeah, in the shower this morning I was thinking that not loading
SPI till after the on_init code runs would alleviate the concerns
about transactionality and permissions --- that would ensure that
whatever on_init does affects only the Perl world and not the database
world.

That's included in the current patch (and also applies to END blocks).

However, we're not out of the woods yet. In a trusted interpreter
(plperl not plperlu), is the on_init code executed before we lock down
the interpreter with Safe? I would think it has to be since the main
point AFAICS is to let you preload code via "use". But then what is
left of the security guarantees of plperl? I can hardly imagine DBAs
wanting to vet a few thousand lines of random Perl code to see if it
contains anything that could be subverted.

plperl.on_perl_init code, set by the DBA, runs before the Safe
compartment is created. Without explicitextra steps the Safe
compartment has no access to code loaded by plperl.on_perl_init.

The Safe compartment (plperl) could get access to loaded code in one of
these ways:
1. by using SQL to call a plperlu function that accesses the code.
2. by the DBA 'sharing' a specific subroutine with the compartment.
3. by the DBA loading a module into the compartment.

There's no formal interface for 2. and 3. at the moment, so the only
official option is 1. (The final patch in the series includes some
building blocks towards an interface for 2 & 3, but doesn't add one.)

If you're willing to also confine the feature to plperlu, then maybe
the risk level could be decreased from insane to merely unreasonable.

I think you could reasonably describe plperl.on_perl_init as effectively
confined to plperlu (because plperl has no access to any new code).

http://archives.postgresql.org/message-id/26766.1263149361@sss.pgh.pa.us
For the record, I think it's a bad idea to run arbitrary
user-defined code in the postmaster, and I think it's a worse idea to
run arbitrary user-defined code at backend shutdown (the END-blocks bit).
I do not care in the least what applications you think this might
enable --- the negative consequences for overall system stability seem
to me to outweigh any possible arguments on that side.
- What happens when the supplied code has errors,

For on_perl_init it throws an exception that propagates to the user
statement that triggered the initialization of perl. It also ensures
that perl is left in a non-initialized state, so any further uses
also fail.

For END blocks an error triggers an exception that's caught by perl.

(As noted above, there's no access to postgres from init or END code.)

- takes an unreasonable amount of time to run,

Unreasonable is in the eye of the DBA, of course, and they
have the discretion to set on_perl_init to fit their needs.

For END blocks, I don't see how this issue is any different from
"users might do something dumb", like DO 'while(1){}' language plperl;
(or plpython , pltcl, or plpgsql for that matter).

- does something unsafe,

Such as? The code can't do anything more unsafe than is already possible.

- depends on the backend not being in an error state already,

The code has no access to postgress, whatever the state.

- etc. etc?

I'd welcome more concrete examples of potential issues.

Tim.

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 12:46:42AM -0700, Alex Hunsaker wrote:

FWIW the atexit scares me to.

In what way, specifically?

It runs too late, and too unpredictably, during the shutdown sequence.
(In particular note that shutdown itself might be fired as an atexit
callback, a move forced on us by exactly the sort of random user code
that you want to add more of. It's not clear whether a Perl-added
atexit would fire before or after that.)

I understand concerns about interacting with the database, so the
patch ensures that any use of spi functions throws an exception.

That assuages my fears to only a tiny degree. SPI is not the only
possible connection between perl code and the rest of the backend.
Indeed, AFAICS the major *point* of these additions is to allow people
to insert unknown other functionality that is likely to interact
with the rest of the backend; a prospect that doesn't make me feel
better about it.

Specifically, how is code that starts executing at the end of a session
different in risk to code that starts executing before the end of a session?

If it runs before the shutdown sequence starts, we know we have a
functioning backend. Once shutdown starts, it's unknown and mostly
untested exactly what subsystems will still work and which won't.
Injecting arbitrary user-written code into an unspecified point in
that sequence is not a recipe for good results.

Lastly, an atexit trigger will still fire during FATAL or PANIC aborts,
which scares me even more. When the house is already afire, it's
not prudent to politely let user-written perl code do whatever it wants
before you get the heck out of there.

regards, tom lane

Tom Lane wrote:

Indeed, AFAICS the major *point* of these additions is to allow people
to insert unknown other functionality that is likely to interact
with the rest of the backend; a prospect that doesn't make me feel
better about it.

No. The major use case we've seen for END blocks is to allow a profiler
to write its data out. That should have zero interaction with the rest
of the backend.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

Indeed, AFAICS the major *point* of these additions is to allow people
to insert unknown other functionality that is likely to interact
with the rest of the backend; a prospect that doesn't make me feel
better about it.

No. The major use case we've seen for END blocks is to allow a profiler
to write its data out. That should have zero interaction with the rest
of the backend.

Really? We've found that gprof, for instance, doesn't exactly have
"zero interaction with the rest of the backend" --- there's actually
a couple of different bits in there to help it along, including a
behavioral change during shutdown. I rather doubt that Perl profilers
would turn out much different.

But in any case, I don't believe for a moment that profiling is the only
or even the largest use to which people would try to put this.

regards, tom lane

On Wed, Jan 27, 2010 at 11:13:43AM -0500, Tom Lane wrote:

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 12:46:42AM -0700, Alex Hunsaker wrote:

FWIW the atexit scares me to.

In what way, specifically?

It runs too late, and too unpredictably, during the shutdown sequence.
(In particular note that shutdown itself might be fired as an atexit
callback, a move forced on us by exactly the sort of random user code
that you want to add more of. It's not clear whether a Perl-added
atexit would fire before or after that.)

man atexit says "Functions so registered are called in reverse order".
Since the plperl atexit is called only when a plperl SP or DO is
executed it would fire before any atexit() registered during startup.

The timing and predictability shouldn't be a significant concern if the
plperl subsystem can't interact with the rest of the backend - which is
the intent.

I understand concerns about interacting with the database, so the
patch ensures that any use of spi functions throws an exception.

That assuages my fears to only a tiny degree. SPI is not the only
possible connection between perl code and the rest of the backend.

Could you give me some examples of others?

Indeed, AFAICS the major *point* of these additions is to allow people
to insert unknown other functionality that is likely to interact
with the rest of the backend; a prospect that doesn't make me feel
better about it.

The major point is *not at all* to allow people to interact with the
rest of the backend. I'm specifically trying to limit that.
The major point is simply to allow perl code to clean itself up properly.

Specifically, how is code that starts executing at the end of a session
different in risk to code that starts executing before the end of a session?

If it runs before the shutdown sequence starts, we know we have a
functioning backend. Once shutdown starts, it's unknown and mostly
untested exactly what subsystems will still work and which won't.
Injecting arbitrary user-written code into an unspecified point in
that sequence is not a recipe for good results.

The plperl subsystem is isolated from, and can't interact with, the rest
of the backend during shutdown.
Can you give me examples where that's not the case?

Lastly, an atexit trigger will still fire during FATAL or PANIC aborts,
which scares me even more. When the house is already afire, it's
not prudent to politely let user-written perl code do whatever it wants
before you get the heck out of there.

Again, that point rests on your underlying concern about interaction
between plperl and the rest of the backend. Examples?

Is there some way for plperl.c to detect a FATAL or PANIC abort?
If so, or if one could be added, then we could skip the END code in
those circumstances.

I don't really want to add more GUCs, but perhaps controlling END
block execution via a plperl.destroy_end=bool (default false) would
help address your concerns.

Tim.

Tom Lane <tgl@sss.pgh.pa.us> writes:

[...]
Lastly, an atexit trigger will still fire during FATAL or PANIC aborts,
which scares me even more. When the house is already afire, it's
not prudent to politely let user-written perl code do whatever it wants
before you get the heck out of there.

Is there a reason that these panics don't use _exit(3) to bypass
atexit hooks?

- FChE

Tom Lane wrote:

But in any case, I don't believe for a moment that profiling is the only
or even the largest use to which people would try to put this.

Well, ISTR there have been requests over the years for event handlers
for (among other things) session shutdown, so if you're speculating that
people would use this as an end run around our lack of such things you
could be right. Maybe providing for such handlers in a more general and
at the same time more safe way would be an alternative.

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

I see I asked the wrong question. Start again.
What more should be done to make all or some of it acceptable?

I think a "must" is to get rid of the use of atexit(). Possibly an
on_proc_exit callback could be used instead, although I'm not sure how
you'd handle the case of code loaded in the postmaster that would like
corresponding exit-time code to happen in child processes. (OTOH, it
seems likely that it's impossible to make that work correctly anyway.
It certainly isn't going to work the same on EXEC_BACKEND platforms
as anywhere else, and I don't particularly want to see us documenting
that the feature works differently on Windows than elsewhere.)

Dropping the ability to make the postmaster run any such code would go a
very long way towards fixing the above, as well as assuaging other
fears.

The other thing that I find entirely unconvincing is Tim's idea that
shutting off SPI isolates perl from the rest of the backend. I have
no confidence in that, but no real idea of how to do better either :-(.
If you think that shutting off SPI is sufficient, you can find
counterexamples in the CVS history, for instance where we had to take
special measures to prevent Perl from screwing up the locale settings.
I'm afraid that on_perl_init is going to vastly expand the opportunities
for that kind of unwanted side-effect; and the earlier that it runs, the
more likely it's going to be that we can't recover easily.

regards, tom lane

fche@redhat.com (Frank Ch. Eigler) writes:

Tom Lane <tgl@sss.pgh.pa.us> writes:

Lastly, an atexit trigger will still fire during FATAL or PANIC aborts,
which scares me even more. When the house is already afire, it's
not prudent to politely let user-written perl code do whatever it wants
before you get the heck out of there.

Is there a reason that these panics don't use _exit(3) to bypass
atexit hooks?

Well, I don't really want to entirely forbid the use of atexit() ---
I'm just concerned about using it to run arbitrary user-written code.
There might be more limited purposes for which it's a reasonable choice.

regards, tom lane

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 11:13:43AM -0500, Tom Lane wrote:

(In particular note that shutdown itself might be fired as an atexit
callback, a move forced on us by exactly the sort of random user code
that you want to add more of. It's not clear whether a Perl-added
atexit would fire before or after that.)

man atexit says "Functions so registered are called in reverse order".
Since the plperl atexit is called only when a plperl SP or DO is
executed it would fire before any atexit() registered during startup.

Right, which means that it would occur either before or after
on_proc_exit processing, depending on whether we got there through
an exit() call or via the normal proc_exit sequence. That's just
the kind of instability I don't want to have to debug.

The plperl subsystem is isolated from, and can't interact with, the rest
of the backend during shutdown.

This is exactly the claim that I have zero confidence in. Quite
frankly, the problem with Perl as an extension language is that Perl was
never designed to be a subsystem: it feels free to mess around with the
entire state of the process. We've been burnt multiple times by that
even with the limited use we make of Perl now, and these proposed
additions are going to make it a lot worse IMO.

regards, tom lane

On Jan 27, 2010, at 9:08 AM, Tom Lane wrote:

This is exactly the claim that I have zero confidence in. Quite
frankly, the problem with Perl as an extension language is that Perl was
never designed to be a subsystem: it feels free to mess around with the
entire state of the process. We've been burnt multiple times by that
even with the limited use we make of Perl now, and these proposed
additions are going to make it a lot worse IMO.

Can you provide an example? Such concerns are impossible to address without concrete examples.

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 9:08 AM, Tom Lane wrote:

This is exactly the claim that I have zero confidence in. Quite
frankly, the problem with Perl as an extension language is that Perl was
never designed to be a subsystem: it feels free to mess around with the
entire state of the process. We've been burnt multiple times by that
even with the limited use we make of Perl now, and these proposed
additions are going to make it a lot worse IMO.

Can you provide an example? Such concerns are impossible to address without concrete examples.

Two examples that I can find in a quick review of our CVS history: perl
stomping on the process's setlocale state, and perl stomping on the
stdio state (Windows only).

regards, tom lane

On Jan 27, 2010, at 10:08 AM, Tom Lane wrote:

Two examples that I can find in a quick review of our CVS history: perl
stomping on the process's setlocale state, and perl stomping on the
stdio state (Windows only).

Are there links to those commits?

Thanks,

David

On Wed, Jan 27, 2010 at 12:08:48PM -0500, Tom Lane wrote:

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 11:13:43AM -0500, Tom Lane wrote:

(In particular note that shutdown itself might be fired as an atexit
callback, a move forced on us by exactly the sort of random user code
that you want to add more of. It's not clear whether a Perl-added
atexit would fire before or after that.)

man atexit says "Functions so registered are called in reverse order".
Since the plperl atexit is called only when a plperl SP or DO is
executed it would fire before any atexit() registered during startup.

Right, which means that it would occur either before or after
on_proc_exit processing, depending on whether we got there through
an exit() call or via the normal proc_exit sequence. That's just
the kind of instability I don't want to have to debug.

Okay. I could change the callback code to ignore calls if
proc_exit_inprogress is false. So an abnormal shutdown via exit()
wouldn't involve plperl at all. (Alternatively I could use use
on_proc_exit() instead of atexit() to register the callback.)

Would that address this call sequence instability issue?

The plperl subsystem is isolated from, and can't interact with, the
rest of the backend during shutdown.

This is exactly the claim that I have zero confidence in. Quite
frankly, the problem with Perl as an extension language is that Perl was
never designed to be a subsystem: it feels free to mess around with the
entire state of the process. We've been burnt multiple times by that
even with the limited use we make of Perl now, and these proposed
additions are going to make it a lot worse IMO.

On Wed, Jan 27, 2010 at 09:53:44AM -0800, David E. Wheeler wrote:

Can you provide an example? Such concerns are impossible to address
without concrete examples.

On Wed, Jan 27, 2010 at 01:08:56PM -0500, Tom Lane wrote:

Two examples that I can find in a quick review of our CVS history: perl
stomping on the process's setlocale state, and perl stomping on the
stdio state (Windows only).

Neither of those relate to the actions of perl source code.
To address that, instead of calling perl_destruct() to perform a
complete destruction I could just execute END blocks and object
destructors. That would avoid executing any system-level actions.

Do you have any examples of how a user could write code in a plperl END
block that would interact with the rest of the backend?

Tim.

On Wed, Jan 27, 2010 at 11:28:02AM -0500, Tom Lane wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

Tom Lane wrote:

Indeed, AFAICS the major *point* of these additions is to allow people
to insert unknown other functionality that is likely to interact
with the rest of the backend; a prospect that doesn't make me feel
better about it.

No. The major use case we've seen for END blocks is to allow a profiler
to write its data out. That should have zero interaction with the rest
of the backend.

Really? We've found that gprof, for instance, doesn't exactly have
"zero interaction with the rest of the backend" --- there's actually
a couple of different bits in there to help it along, including a
behavioral change during shutdown. I rather doubt that Perl profilers
would turn out much different.

Devel::NYTProf (http://blog.timbunce.org/tag/nytprof/) has zero
interaction with the rest of the backend.

It works in PostgreSQL 8.4, although greatly handicapped by the lack of
END blocks. http://search.cpan.org/perldoc?Devel::NYTProf::PgPLPerl

But in any case, I don't believe for a moment that profiling is the only
or even the largest use to which people would try to put this.

Can you give any examples?

Tim.

On Jan 27, 2010, at 1:27 PM, Tim Bunce wrote:

Okay. I could change the callback code to ignore calls if
proc_exit_inprogress is false. So an abnormal shutdown via exit()
wouldn't involve plperl at all. (Alternatively I could use use
on_proc_exit() instead of atexit() to register the callback.)

Given Tom's hesitace about atexit(), perhaps that would be best.

Neither of those relate to the actions of perl source code.
To address that, instead of calling perl_destruct() to perform a
complete destruction I could just execute END blocks and object
destructors. That would avoid executing any system-level actions.

Does perl_destruct() execute system-level actions, then? If so, then it seems prudent to either audit such actions or, as you say, call destructors directly.

Do you have any examples of how a user could write code in a plperl END
block that would interact with the rest of the backend?

I appreciate you taking the time to look at ways to address the issues Tom has raised, Tim. Good on you.

There is so much benefit to this level of interaction, as shown by the success of mod_perl and other forking environments that support pre-loading code, that I think it'd be extremely valuable to get these features in 9.0. They really allow Perl to be a first-class PL in a way that it wasn't before.

So if there is no way other than SPI for Perl code to interact with the backend, and system-level actions in Perl itself are disabled, it seems to me that the major issues are addressed. Am I wrong, Tom?

Best,

David

On Wed, Jan 27, 2010 at 4:51 PM, David E. Wheeler <david@kineticode.com> wrote:

Neither of those relate to the actions of perl source code.
To address that, instead of calling perl_destruct() to perform a
complete destruction I could just execute END blocks and object
destructors. That would avoid executing any system-level actions.

Does perl_destruct() execute system-level actions, then? If so, then it seems prudent to either audit such actions or, as you say, call destructors directly.

What exactly do we mean by "system-level actions"? I mean, END blocks
can execute arbitrary code....

...Robert

On Jan 27, 2010, at 1:53 PM, Robert Haas wrote:

What exactly do we mean by "system-level actions"? I mean, END blocks
can execute arbitrary code....

Yeah. In Perl. What part of Perl can access the backend systems without SPI? And that it couldn't do at any other point in runtime?

Best,

David

On Wed, Jan 27, 2010 at 01:51:47PM -0800, David E. Wheeler wrote:

On Jan 27, 2010, at 1:27 PM, Tim Bunce wrote:

Okay. I could change the callback code to ignore calls if
proc_exit_inprogress is false. So an abnormal shutdown via exit()
wouldn't involve plperl at all. (Alternatively I could use use
on_proc_exit() instead of atexit() to register the callback.)

Given Tom's hesitace about atexit(), perhaps that would be best.

I've a draft patch using !proc_exit_inprogress implemented now
(appended) and I'll test it tomorrow. That was the simplest to do first.
Once I've a reasonable way of testing the exit() and proc_exit() code
paths I'll try using on_proc_exit().

Neither of those relate to the actions of perl source code.
To address that, instead of calling perl_destruct() to perform a
complete destruction I could just execute END blocks and object
destructors. That would avoid executing any system-level actions.

Does perl_destruct() execute system-level actions, then?

Potentially: Kills threads it knows about (should be none), wait for
children (should be none), flushes all open *perl* file handles (should
be none for plperl), tears down PerlIO layers, etc. etc. In practice
none of that should affect the backend, but it's possible, especially
for the Windows port. Since none of that is needed it can be skipped.

If so, then it seems prudent to either audit such actions or, as you
say, call destructors directly.

The patch now just calls END blocks and DESTROY methods.

Do you have any examples of how a user could write code in a plperl END
block that would interact with the rest of the backend?

I appreciate you taking the time to look at ways to address the issues
Tom has raised, Tim. Good on you.

Thanks David. I appreciate the visible support!

Tom and the team set the bar high, rightly, so it's certainly been a
challenging introduction to PostgreSQL development!

Tim.

diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 8315d5a..38f2d35 100644
*** a/src/pl/plperl/plperl.c
--- b/src/pl/plperl/plperl.c
***************
*** 27,32 ****
--- 27,33 ----
  #include "miscadmin.h"
  #include "nodes/makefuncs.h"
  #include "parser/parse_type.h"
+ #include "storage/ipc.h"
  #include "utils/builtins.h"
  #include "utils/fmgroids.h"
  #include "utils/guc.h"
*************** _PG_init(void)
*** 281,287 ****
  static void
  plperl_fini(void)
  {
! 	plperl_ending = true;
  	plperl_destroy_interp(&plperl_trusted_interp);
  	plperl_destroy_interp(&plperl_untrusted_interp);
  	plperl_destroy_interp(&plperl_held_interp);
--- 282,297 ----
  static void
  plperl_fini(void)
  {
! 	plperl_ending = true; /* disables use of spi_* functions */
! 
! 	/*
! 	 * Only perform perl cleanup if we're exiting cleanly via proc_exit().
! 	 * If proc_exit_inprogress is false then exit() was called directly
! 	 * (because we call atexit() very late, so get called early).
! 	 */
! 	if (!proc_exit_inprogress)
! 		return;
! 
  	plperl_destroy_interp(&plperl_trusted_interp);
  	plperl_destroy_interp(&plperl_untrusted_interp);
  	plperl_destroy_interp(&plperl_held_interp);
*************** plperl_destroy_interp(PerlInterpreter **
*** 595,602 ****
  {
  	if (interp && *interp)
  	{
! 		perl_destruct(*interp);
! 		perl_free(*interp);
  		*interp = NULL;
  	}
  }
--- 605,640 ----
  {
  	if (interp && *interp)
  	{
! 		/*
! 		 * Only a very minimal destruction is performed.
! 		 * Just END blocks and object destructors, no system-level actions.
! 		 * Code code here extracted from perl's perl_destruct().
! 		 */
! 
! 		/* Run END blocks */
! 		if (PL_exit_flags & PERL_EXIT_DESTRUCT_END) {
! 			dJMPENV;
! 			int x = 0;
! 
! 			JMPENV_PUSH(x);
! 			PERL_UNUSED_VAR(x);
! 			if (PL_endav && !PL_minus_c)
! 				call_list(PL_scopestack_ix, PL_endav);
! 			JMPENV_POP;
! 		}
! 		LEAVE;
! 		FREETMPS;
! 
! 		PL_dirty = TRUE;
! 
! 		/* destroy objects - call DESTROY methods */
! 		if (PL_sv_objcount) {
! 			Perl_sv_clean_objs(aTHX);
! 			PL_sv_objcount = 0;
! 			if (PL_defoutgv && !SvREFCNT(PL_defoutgv))
! 				PL_defoutgv = NULL; /* may have been freed */
! 		}
! 
  		*interp = NULL;
  	}
  }

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 1:53 PM, Robert Haas wrote:

What exactly do we mean by "system-level actions"? I mean, END blocks
can execute arbitrary code....

Yeah. In Perl. What part of Perl can access the backend systems without SPI? And that it couldn't do at any other point in runtime?

You still aren't letting go of the notion that Perl could only affect
the rest of the backend via SPI. The point I'm trying to impress on you
is that there are any number of other possible pathways, and that Perl's
historical assumption that it owns all resources of the process make
those pathways a nontrivial hazard. Anything that Perl does to libc
state, open file handles, etc etc carries a high risk of breaking the
backend.

Now it is certainly true that any such hazards can be created just from
use of plperlu (we hope only plperlu, and not plperl ...) today,
without any use of the proposed additional features. What is bothering
me about these features is that their entire reason for existence is to
encourage people to use parts of Perl that have time-extended effects on
the process state. That means that (a) the probability of problems goes
up substantially, and (b) our ability to fix such problems goes down
substantially. Right now, the canonical approach to trying to undo
anything bad Perl does is to save/restore process state around a plperl
call. If we're trying to support usages in which Perl has time-extended
effects on process state, that solution goes out the window, and we have
to think of some other way to coexist with Perl. (Where, I note,
"coexist" means "Perl does what it damn pleases and we have to pick up
the pieces" --- we're not likely to get any cooperation on limiting
damage from that side. Nobody even suggested that we treat stomping on
setlocale state as a Perl bug, for example, rather than a fact of life
that we just had to work around however we could.)

So the real bottom line here is that I foresee this patch as being
destabilizing and requiring us to put large amounts of time into
figuring out workarounds for whatever creative things people decide to
try to do with Perl. I'd feel better about it if I thought that we
could get away with a policy of "if it breaks it's your problem", but
I do not think that will fly from a PR standpoint. It hasn't in the
past.

regards, tom lane

Tim Bunce <Tim.Bunce@pobox.com> writes:

Okay. I could change the callback code to ignore calls if
proc_exit_inprogress is false. So an abnormal shutdown via exit()
wouldn't involve plperl at all. (Alternatively I could use use
on_proc_exit() instead of atexit() to register the callback.)

Use on_proc_exit please. I will continue to object to any attempt
to hang arbitrary processing on atexit().

An advantage of on_proc_exit from your end is that it should allow
you to not have to try to prevent the END blocks from using SPI,
as that would still be perfectly functional when your callback
gets called. (Starting a new transaction would be a good idea
though, cf Async_UnlistenOnExit.)

regards, tom lane

On Jan 27, 2010, at 3:11 PM, Tom Lane wrote:

You still aren't letting go of the notion that Perl could only affect
the rest of the backend via SPI. The point I'm trying to impress on you
is that there are any number of other possible pathways, and that Perl's
historical assumption that it owns all resources of the process make
those pathways a nontrivial hazard. Anything that Perl does to libc
state, open file handles, etc etc carries a high risk of breaking the
backend.

As could any other code that executes then, including C libraries installed from pgFoundry and loaded by a DBA.

Now it is certainly true that any such hazards can be created just from
use of plperlu (we hope only plperlu, and not plperl ...) today,
without any use of the proposed additional features. What is bothering
me about these features is that their entire reason for existence is to
encourage people to use parts of Perl that have time-extended effects on
the process state.

Well, mainly it's to avoid the overhead of loading the code except at startup.

That means that (a) the probability of problems goes
up substantially,

Why? Arbitrary code can already execute at start time. Is Perl special somehow?

and (b) our ability to fix such problems goes down
substantially.

Why is it your problem?

Right now, the canonical approach to trying to undo
anything bad Perl does is to save/restore process state around a plperl
call. If we're trying to support usages in which Perl has time-extended
effects on process state, that solution goes out the window, and we have
to think of some other way to coexist with Perl. (Where, I note,
"coexist" means "Perl does what it damn pleases and we have to pick up
the pieces" --- we're not likely to get any cooperation on limiting
damage from that side. Nobody even suggested that we treat stomping on
setlocale state as a Perl bug, for example, rather than a fact of life
that we just had to work around however we could.)

How is that different from any other code that gets loaded when the server starts, exactly?

Do, however, feel free to report Perl bugs. Just run `perlbug`.

So the real bottom line here is that I foresee this patch as being
destabilizing and requiring us to put large amounts of time into
figuring out workarounds for whatever creative things people decide to
try to do with Perl. I'd feel better about it if I thought that we
could get away with a policy of "if it breaks it's your problem", but
I do not think that will fly from a PR standpoint. It hasn't in the
past.

mod_perl has for many years. Provide lots of caveats in the documentation. Point users to it when they write in about a problem.

Truth is, the vast majority of Perl modules are pretty well-behaved. I sincerely doubt you'd hear much complaint. Have the Apache guys had to take any special steps to protect httpd from mod_perl?

Best,

David

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 11:28:02AM -0500, Tom Lane wrote:

Really? We've found that gprof, for instance, doesn't exactly have
"zero interaction with the rest of the backend" --- there's actually
a couple of different bits in there to help it along, including a
behavioral change during shutdown. I rather doubt that Perl profilers
would turn out much different.

Devel::NYTProf (http://blog.timbunce.org/tag/nytprof/) has zero
interaction with the rest of the backend.

I don't have to read any further than the place where it says "doesn't
work if you call both plperl and plperlu" to realize that that's quite
false. Maybe we have different definitions of what a software
interaction is...

regards, tom lane

On Jan 27, 2010, at 3:33 PM, Tom Lane wrote:

I don't have to read any further than the place where it says "doesn't
work if you call both plperl and plperlu" to realize that that's quite
false. Maybe we have different definitions of what a software
interaction is...

I think that dates from when plperl and plperlu couldn't co-exists, which was fixed a few months ago, n'est pas?

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 3:33 PM, Tom Lane wrote:

I don't have to read any further than the place where it says "doesn't
work if you call both plperl and plperlu" to realize that that's quite
false. Maybe we have different definitions of what a software
interaction is...

I think that dates from when plperl and plperlu couldn't co-exists, which was fixed a few months ago, n'est pas?

No, that was fixed years ago, at least if you have a modern Perl build
that supports multiplicity at all.

regards, tom lane

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 3:11 PM, Tom Lane wrote:

... Anything that Perl does to libc
state, open file handles, etc etc carries a high risk of breaking the
backend.

As could any other code that executes then, including C libraries installed from pgFoundry and loaded by a DBA.

Absolutely. The difference here is in who is going to be expected to
try to deal with any problems. When somebody says "if I do this in
plperlu, my database crashes! Postgres sux!" it's not going to help to
say "that's a Perl bug", even if an independent observer might agree.
It's going to be *our* problem, and I don't see any reason to expect
a shred of help from the Perl side.

regards, tom lane

On Jan 27, 2010, at 4:10 PM, Tom Lane wrote:

Absolutely. The difference here is in who is going to be expected to
try to deal with any problems. When somebody says "if I do this in
plperlu, my database crashes! Postgres sux!" it's not going to help to
say "that's a Perl bug", even if an independent observer might agree.
It's going to be *our* problem, and I don't see any reason to expect
a shred of help from the Perl side.

Is that not the case with plperlu already?

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 4:10 PM, Tom Lane wrote:

Absolutely. The difference here is in who is going to be expected to
try to deal with any problems. When somebody says "if I do this in
plperlu, my database crashes! Postgres sux!" it's not going to help to
say "that's a Perl bug", even if an independent observer might agree.
It's going to be *our* problem, and I don't see any reason to expect
a shred of help from the Perl side.

Is that not the case with plperlu already?

Sure. Which is why I'm resisting expanding our exposure to it.

regards, tom lane

Tom Lane wrote:

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 3:33 PM, Tom Lane wrote:

I don't have to read any further than the place where it says "doesn't
work if you call both plperl and plperlu" to realize that that's quite
false. Maybe we have different definitions of what a software
interaction is...

I think that dates from when plperl and plperlu couldn't co-exists, which was fixed a few months ago, n'est pas?

No, that was fixed years ago, at least if you have a modern Perl build
that supports multiplicity at all.

To be strictly accurate, what we fixed years ago was that we used to run
plperl and plperlu in the same interpreter, and that caused all sorts of
information leaks, so we switched to running in two interpreters, which
in turn became a problem for perl builds that didn't define multiplicity.

The problem here is that NYTprof is apparently not multiplicity safe. I
guess the question is what would happen if you tried to load it with
both plperl and plperlu. In any case, it's a known and documented issue,
so it's not one I'd be terribly worried about.

cheers

andrew

On Jan 27, 2010, at 4:15 PM, Tom Lane wrote:

Is that not the case with plperlu already?

Sure. Which is why I'm resisting expanding our exposure to it

I don't understand how it's expanding core's exposure to it.

Best,

David

"David E. Wheeler" <david@kineticode.com> writes:

On Jan 27, 2010, at 4:15 PM, Tom Lane wrote:

Sure. Which is why I'm resisting expanding our exposure to it

I don't understand how it's expanding core's exposure to it.

[ shrug...] I see little point in repeating myself yet again.
It's obvious that the people who want this are entirely willing
to adopt a Pollyanna-ishly optimistic view about its potential
to cause serious problems that we may or may not be able to fix.

I don't really expect to be able to prevent something along this line
from getting committed --- I'm merely hoping to circumscribe it as much
as possible and get large WARNING items into the manual's description.

regards, tom lane

On Jan 27, 2010, at 4:33 PM, Tom Lane wrote:

[ shrug...] I see little point in repeating myself yet again.
It's obvious that the people who want this are entirely willing
to adopt a Pollyanna-ishly optimistic view about its potential
to cause serious problems that we may or may not be able to fix.

Well, no. The problems you raise already exist in plperlu. And I would argue that they're worse there, as the DBA can give others permission to create PL/PerlU functions, and those users can do all kinds of crazy shit with them. on_perl_init can be executed the DBA only. It's scope is far less. This is *safe* than PL/PerlU, while given more capability to PL/Perl.

I don't really expect to be able to prevent something along this line
from getting committed --- I'm merely hoping to circumscribe it as much
as possible and get large WARNING items into the manual's description.

Oh, absolutely. Your sober attention to security issues is greatly appreciated by us fanboys.

Best,

David

PS: I'm a PostgreSQL fanboy, not a Tom Lane fanboy. ;-P

Tom Lane wrote:

It's obvious that the people who want this are entirely willing
to adopt a Pollyanna-ishly optimistic view about its potential
to cause serious problems that we may or may not be able to fix.

I don't really expect to be able to prevent something along this line
from getting committed --- I'm merely hoping to circumscribe it as much
as possible and get large WARNING items into the manual's description.

Well, we seem to have got much closer to what you can live with on the
END block issue, although we took a rather roundabout route to get there.

Perhaps with a little more work we can achieve something similar on the
on_perl_init front (which, in any case, I don't regard as being as
important as the END blocks, although Tim might not agree.)

cheers

andrew

On Wed, Jan 27, 2010 at 06:33:19PM -0500, Tom Lane wrote:

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 11:28:02AM -0500, Tom Lane wrote:

Really? We've found that gprof, for instance, doesn't exactly have
"zero interaction with the rest of the backend" --- there's actually
a couple of different bits in there to help it along, including a
behavioral change during shutdown. I rather doubt that Perl profilers
would turn out much different.

Devel::NYTProf (http://blog.timbunce.org/tag/nytprof/) has zero
interaction with the rest of the backend.

I don't have to read any further than the place where it says "doesn't
work if you call both plperl and plperlu" to realize that that's quite
false.

NYTProf is not, currently, multiplicity-safe. That's a limitation I
intend to fix.

Maybe we have different definitions of what a software interaction is...

Doing _anything_ in the backend is an interaction of some kind, e.g.,
shifting later memory allocations to a different address. But that's not
a very practical basis for a definition.

From what you said, quoted above, it seemed that your definition of
"interaction with the rest of the backend" was more much more direct.
The specific example you gave related to the backend code needing to be
modified to support the gprof profiler. Clearly that's not the case for
NYTProf.

We're splitting hairs now.

Tim.

On Wed, Jan 27, 2010 at 06:27:50PM -0500, Tom Lane wrote:

Tim Bunce <Tim.Bunce@pobox.com> writes:

Okay. I could change the callback code to ignore calls if
proc_exit_inprogress is false. So an abnormal shutdown via exit()
wouldn't involve plperl at all. (Alternatively I could use use
on_proc_exit() instead of atexit() to register the callback.)

Use on_proc_exit please. I will continue to object to any attempt
to hang arbitrary processing on atexit().

Ok.

An advantage of on_proc_exit from your end is that it should allow
you to not have to try to prevent the END blocks from using SPI,
as that would still be perfectly functional when your callback
gets called. (Starting a new transaction would be a good idea
though, cf Async_UnlistenOnExit.)

I'm surprised that you're suggesting that END block should be allowed to
interact with the backend via SPI. It seems to go against what you've
said previously about code running at shutdown.

I've no use-case for that so I'm happy to leave it disabled. If someone
does have a sane use-case, please let me know. It can always be enabled later.

Tim.

Tim Bunce wrote:

I've no use-case for that so I'm happy to leave it disabled. If someone
does have a sane use-case, please let me know. It can always be enabled later.

As I noted upthread, there have been requests for user level session end
handlers before. With SPI enabled as Tom suggests, this would just about
buy us that for free.

But if you're uncomfortable about it we can take that up at a later date.

cheers

andrew

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 06:27:50PM -0500, Tom Lane wrote:

An advantage of on_proc_exit from your end is that it should allow
you to not have to try to prevent the END blocks from using SPI,
as that would still be perfectly functional when your callback
gets called. (Starting a new transaction would be a good idea
though, cf Async_UnlistenOnExit.)

I'm surprised that you're suggesting that END block should be allowed to
interact with the backend via SPI. It seems to go against what you've
said previously about code running at shutdown.

I think you have completely misunderstood what I'm complaining about.
What I'm not happy about is executing operations at a point where
they're likely to be ill-defined because the code is in the wrong state.
In an early on_proc_exit hook, the system is for all practical purposes
still fully functional, and so I don't see a reason for an arbitrary
restriction on what the END blocks should be able to do.

(Or, to repeat myself in a different way: the no-SPI restriction is
utterly useless to guard against my real concerns anyway. I see no
point in it either here or elsewhere.)

regards, tom lane

On Thu, Jan 28, 2010 at 10:39:33AM -0500, Tom Lane wrote:

Tim Bunce <Tim.Bunce@pobox.com> writes:

On Wed, Jan 27, 2010 at 06:27:50PM -0500, Tom Lane wrote:

An advantage of on_proc_exit from your end is that it should allow
you to not have to try to prevent the END blocks from using SPI,
as that would still be perfectly functional when your callback
gets called. (Starting a new transaction would be a good idea
though, cf Async_UnlistenOnExit.)

I'm surprised that you're suggesting that END block should be allowed to
interact with the backend via SPI. It seems to go against what you've
said previously about code running at shutdown.

I think you have completely misunderstood what I'm complaining about.
What I'm not happy about is executing operations at a point where
they're likely to be ill-defined because the code is in the wrong state.
In an early on_proc_exit hook, the system is for all practical purposes
still fully functional, and so I don't see a reason for an arbitrary
restriction on what the END blocks should be able to do.

Ah, okay. I guess I missed your underlying concerns in:

http://archives.postgresql.org/message-id/26766.1263149361@sss.pgh.pa.us
For the record, [...] and I think it's a worse idea to run
arbitrary user-defined code at backend shutdown (the END-blocks bit).

(Or, to repeat myself in a different way: the no-SPI restriction is
utterly useless to guard against my real concerns anyway. I see no
point in it either here or elsewhere.)

I've left it in the updated patch I've just posted.
There are two more plperl patches in the current commitfest that I'd
like to chaperone through to commit (in some form or other) first.

Thanks for your help Tom.

Tim.