Re: Hot Standy introduced problem with query cancel behavior

On Thursday 31 December 2009 01:09:57 Robert Haas wrote:
> On Wed, Dec 30, 2009 at 6:43 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> On Wed, Dec 30, 2009 at 2:06 PM, Andres Freund <andres(at)anarazel(dot)de>
wrote:
> >> > On Wednesday 30 December 2009 01:13:01 Simon Riggs wrote:
> >> > > On Tue, 2009-12-29 at 11:13 -0500, Tom Lane wrote:
> >> > > > Andres Freund <andres(at)anarazel(dot)de> writes:
> >> > > > > On Tuesday 29 December 2009 16:22:54 Tom Lane wrote:
> >> > > > > > This seems like a fairly bad idea. One of the intended
> >> > > > > > use-cases is to be able to manually "kill -INT" a misbehaving
> >> > > > > > backend. Assuming that there will be valid info about the
> >> > > > > > signal in shared memory will break that.
> >> > > > >
> >> > > > > Well. That already is the case now. MyProc->recoveryConflictMode
> >> > > > > is checked to recognize what kind of conflict is being
> >> > > > > resolved...
> >> > > >
> >> > > > In that case, HS has already broken it, and we need to fix it not
> >> > > > make it worse.
> >> > > >
> >> > > > My humble opinion is that SIGINT should not be overloaded with
> >> > > > multiple meanings. We already have a multiplexed signal
> >> > > > mechanism, which is what should be used for any additional signal
> >> > > > reasons HS may need to introduce.
> >> > >
> >> > > It's a revelation to me, but yes, I see it now and agree.
> >> > >
> >> > > I'm looking at Fujii-san's multiplexing patch from Jul 31 to rewrite
> >> > > this code using that mechanism. It sounds like it's a neat fit and
> >> > > it should get around the bug report from Kris also if it all works.
> >> >
> >> > Hm. I just read a bit of that multiplexing facility (out of a
> >> > different reason) and I have some doubt about it being used unmodified
> >> > for canceling backends:
> >> >
> >> > procsignal.c:
> >> > /*
> >> > * Note: Since there's no locking, it's possible that the target
> >> > * process detaches from shared memory and exits right after this
> >> > * test, before we set the flag and send signal. And the signal slot
> >> > * might even be recycled by a new process, so it's remotely possible
> >> > * that we set a flag for a wrong process. That's OK, all the signals
> >> > * are such that no harm is done if they're mistakenly fired.
> >> > */
> >> > procsignal.h:
> >> > ...
> >> > * Also, because of race conditions, it's important that all the
> >> > signals be * defined so that no harm is done if a process mistakenly
> >> > receives one. */
> >> >
> >> > When cancelling a backend that behaviour could be a bit annoying ;-)
> >> >
> >> > I guess locking procarray during sending the signal should be enough?
> >>
> >> I think the idea is that you define the behavior of the signal to be
> >> "look at this other piece of state to see whether you should cancel
> >> yourself" rather than just "cancel yourself". Then if a signal is
> >> delivered by mistake, it's no big deal - you just look at the other
> >> piece of state and decide that you don't need to do anything.
> >
> > I dont see an easy way to pass enough information right now. You cant
> > regenerate enough of it in the to be killed backend as most of the
> > relevant information is only available in the startup process. Inventing
> > yet another segment in shm just for this seems overcomplicated to me.
> > Thats why I suggested locking the procarray for this - without having
> > looked at the code that should prevent a backend slot from beimg reused.
> Yeah, I understand, but I have a feeling that the code doesn't do it
> that way right now for a reason. Someone who understands this better
> than I should comment, but I'm thinking you would likely need to lock
> the ProcArray in CheckProcSignal as well, and I'm thinking that can't
> be safely done from within a signal handler.
I don't think we would need to lock in the signal handler.
The situation is that two different flags (at this point at least) are needed.
FATAL which aborts the session and ERROR which aborts the transaction.

Consider the following scenario:
- both flag are set while holding a lock on the procarray
- starting a new backend requires a lock on the procarray
- backend startup cleans up both flags in its ProcSignalSlot (only that specific
one as its the only one manipulated under a lock, mind you)
- transaction startup clears the ERROR flag under locking
- after the new backend started no signal handler targeted for the old backend
can get triggered (new pid, if we consider direct reuse of the same pid we
have much bigger problems anyway), thus no flag targeted for the old backend
can get set

That would require a nice comment explaining that but it should work.

Another possibility would be to make the whole signal delivery mechanism safe
- that should be possible if we had a atomically settable backend id...

Unfortunately that would limit the max lifetime for a backend a bit - as
sig_atomic_t is 4byte on most platforms. So no backend would be allowed to
live after creation of the 2**32-1th backend after it.
I don't immediately see a way circumvent that 32bit barrier without using
assembler or locks.

Andres

PS: Hm. For my former message beeing written on a mobile phone it looked
surprisingly clean...