Re: Add on_perl_init and proper destruction to plperl [PATCH]

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;
}
}