Re: Re: [COMMITTERS] pgsql: Add pg_terminate_backend() to allow terminating only a single - Mailing list pgsql-hackers

Tom Lane wrote:
> Bruce Momjian <bruce@momjian.us> writes:
> > Tom Lane wrote:
> >> A more interesting question is what makes you think that taking
> >> ProcArrayLock here has any value whatsoever.
> 
> > I took the lock so I would be sure the PGPROC array was the matching pid
> > and not some other pid that was created between my check and the setting
> > of the variable.
> 
> Unfortunately, that doesn't work at all, even disregarding the fact that
> you forgot to make proc.c clear the flag when setting up a new process's
> PGPROC.

I assume the PGPROC was cleared with zeros on start.  I can fix that.

> The race condition would go away if ProcArrayRemove zeroed the pid
> field, but I'm afraid that that might break something else; in
> particular I think we still need to be able to manipulate LWLocks after
> ProcArrayRemove, so I suspect this is not safe either.

Oh, good point.  The PGPROC might be dead already but the pid is still
there.  Don't we mark it dead somehow?  Is it only because it isn't in
ProcArrayStruct that it is dead?

> I think the only really safe way to do it is to make a new procarray.c
> function that searches like BackendPidGetProc, but hands the proc back
> with the ProcArrayLock still held.  Or maybe we should just redefine
> BackendPidGetProc that way.

Yea, we could do that and it would clean up my code. 
BackendPidGetProc() doesn't get called many places.  I was a little
worried of looping over ProcArrayStruct while holding an exclusive
lock.

> There are other race conditions in this patch too; notably that lots of
> places feel free to clear QueryCancelPending on-the-fly, thus possibly
> letting them disregard a terminate signal.

True.

> Even assuming that we can fix the garden-variety bugs like these,
> there's still a fundamental problem that an uncooperative user function
> (particularly one in plperl/pltcl/plpython) can indefinitely delay
> response to pg_terminate_backend.  Maybe that's okay, seeing that it can
> similarly hold off or disregard QueryCancel, but I'm not sure the people
> asking for this are really gonna be satisfied.  (One thing we should
> consider is making ERRCODE_ADMIN_SHUTDOWN unconditionally untrappable
> by plpgsql exception blocks, which'd at least fix the issue for plpgsql
> functions.)

They are going to be more satisfied than they are now.  This is the
first listed Admin TODO item.  Every other database allows this simple
capability and we should support it.  Cancel is not the same as exit.

> I'm also thinking that this doesn't fix the problem that we're relying
> on die() to not leave shared memory in a bad state.  All it does is
> restrict things so that we only try that from the main loop rather than
> at any random CHECK_FOR_INTERRUPTS.  That certainly reduces the odds of
> hitting a bug of this type, but I don't see that it can be said to make
> them zero.

Well, OK, can we use proc_exit() instead?  That seems fine too and
probably better than die().

> All in all, this patch wasn't ready to apply without review.  I'm
> currently looking to see whether it's salvageable or not.

OK.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +