Re: Hot Standy introduced problem with query cancel behavior

The JDBC driver's regression test suite has revealed a change in behavior
introduced by the hot standy patch. Previously when a client sent a
cancel request on an idle connection, nothing happened. Now it sends an
error message "ERROR: canceling statement due to user request". This
confuses the driver's protocol handling and it thinks that the error
message is for the next statement issued.

Attached is a test case.

Kris Jurka

On Sat, 2009-12-26 at 20:15 -0500, Kris Jurka wrote:

> The JDBC driver's regression test suite has revealed a change in behavior
> introduced by the hot standy patch. Previously when a client sent a
> cancel request on an idle connection, nothing happened. Now it sends an
> error message "ERROR: canceling statement due to user request". This
> confuses the driver's protocol handling and it thinks that the error
> message is for the next statement issued.
>
> Attached is a test case.

Thanks for the report. I'll see about a fix.

--
Simon Riggs www.2ndQuadrant.com

On Sun, Dec 27, 2009 at 10:42 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> Thanks for the report. I'll see about a fix.

In the end we are about to use SIGINT for two use cases:

- cancel an idle transaction
- cancel a running query

Previously a backend that was DoingCommandRead == true didn't do
anything upon reception of SIGINT, now it aborts either the running
query or the idle transaction, which is why Kris's example behaves
differently now.

If we use the same signal for both cases, the receiving backend cannot
tell what the intention of the sending backend was. That's why I
proposed to make SIGINT similar to SIGUSR1 where we write a reason to
a shared memory structure first and then send the signal (see
http://archives.postgresql.org/pgsql-hackers/2009-12/msg02067.php from
a few days ago).

There was also some dicussion about how to communicate the
cancellation back to the client when its idle transaction got aborted.
I implemented what I thought was the conclusion of the discussion but
haven't received a reply on it yet.

Joachim

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> If we use the same signal for both cases, the receiving backend cannot
> tell what the intention of the sending backend was. That's why I
> proposed to make SIGINT similar to SIGUSR1 where we write a reason to
> a shared memory structure first and then send the signal (see
> http://archives.postgresql.org/pgsql-hackers/2009-12/msg02067.php from
> a few days ago).

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.

regards, tom lane

On Tuesday 29 December 2009 16:22:54 Tom Lane wrote:
> Joachim Wieland <joe(at)mcknight(dot)de> writes:
> > If we use the same signal for both cases, the receiving backend cannot
> > tell what the intention of the sending backend was. That's why I
> > proposed to make SIGINT similar to SIGUSR1 where we write a reason to
> > a shared memory structure first and then send the signal (see
> > http://archives.postgresql.org/pgsql-hackers/2009-12/msg02067.php from
> > a few days ago).
> 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...

Andres

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.

regards, tom lane

On Tue, 2009-12-29 at 14:14 +0100, Joachim Wieland wrote:

> haven't received a reply on it yet.

Apologies for that. I replied today, just forgot to hit send until now.

Thanks again for the patch.

--
Simon Riggs www.2ndQuadrant.com

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.

--
Simon Riggs www.2ndQuadrant.com

On Tue, Dec 29, 2009 at 4:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.

I never intended to change the current behavior.

Actually I wanted to even enhance it by allowing to also cancel an idle
transaction via SIGINT. We are free to define what should happen if there is no
internal reason available because the signal has been sent manually.

We can also use SIGUSR1 of course but then you cannot cancel an idle
transaction just from the commandline. Not sure if this is necessary
though but I would have liked it in the past already (I once used a version of
slony that left transactions idle from time to time...)

Joachim

On Wed, 2009-12-30 at 12:05 +0100, Joachim Wieland wrote:
> On Tue, Dec 29, 2009 at 4:22 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.
>
> I never intended to change the current behavior.
>
> Actually I wanted to even enhance it by allowing to also cancel an idle
> transaction via SIGINT. We are free to define what should happen if there is no
> internal reason available because the signal has been sent manually.
>
> We can also use SIGUSR1 of course but then you cannot cancel an idle
> transaction just from the commandline. Not sure if this is necessary
> though but I would have liked it in the past already (I once used a version of
> slony that left transactions idle from time to time...)

Andres mentioned this in relation to Startup process sending signals to
backends. Startup needs to in-some cases issue a FATAL error also, which
is a separate issue from the discusion around SIGINT.

I will rework the FATAL case and continue to support you in finding a
solution to the cancel-while-idle case.

--
Simon Riggs www.2ndQuadrant.com

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?

Andres

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.

...Robert

----- Ursprüngliche Mitteilung -----
> 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.

Andres

On Wed, Dec 30, 2009 at 6:43 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> ----- Ursprüngliche Mitteilung -----
>> 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.

...Robert

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...

On Wed, 2009-12-30 at 20:06 +0100, Andres Freund wrote:

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

Reading comments alone doesn't show the full situation here.

Before we send signal we check pid also, so the chances of this
happening are fairly small. i.e. we would need to have a backend slot
reused by a new backend with exactly same pid as the last slot holder.

I'm proposing to use this for killing transactions and connections, so I
don't think there's too much harm done there. If the backend is leaving
anyway, that's what we wanted. If its a new guy that is wearing the same
boots then a little collateral damage doesn't leave the server in a bad
place. HS cancellations aren't yet so precise that this matters.

--
Simon Riggs www.2ndQuadrant.com

On Thursday 31 December 2009 12:25:19 Simon Riggs wrote:
> On Wed, 2009-12-30 at 20:06 +0100, Andres Freund wrote:
> > 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 ;-)
>
> Reading comments alone doesn't show the full situation here.
>
> Before we send signal we check pid also, so the chances of this
> happening are fairly small. i.e. we would need to have a backend slot
> reused by a new backend with exactly same pid as the last slot holder.
Well. The problem does not occur for critical errors (i.e. session death)
only. As signal delivery is asynchronous it can very well happen for
transaction cancellation as well.

> I'm proposing to use this for killing transactions and connections, so I
> don't think there's too much harm done there. If the backend is leaving
> anyway, that's what we wanted. If its a new guy that is wearing the same
> boots then a little collateral damage doesn't leave the server in a bad
> place. HS cancellations aren't yet so precise that this matters.
Building racy infrastructure when it can be avoided with a little care still
seems not to be the best path to me.

Andres

On Thu, 2009-12-31 at 13:14 +0100, Andres Freund wrote:
> > > When cancelling a backend that behaviour could be a bit
> annoying ;-)
> >
> > Reading comments alone doesn't show the full situation here.
> >
> > Before we send signal we check pid also, so the chances of this
> > happening are fairly small. i.e. we would need to have a backend
> slot
> > reused by a new backend with exactly same pid as the last slot
> holder.
> Well. The problem does not occur for critical errors (i.e. session
> death)
> only. As signal delivery is asynchronous it can very well happen for
> transaction cancellation as well.

> > I'm proposing to use this for killing transactions and connections,
> so I
> > don't think there's too much harm done there. If the backend is
> leaving
> > anyway, that's what we wanted. If its a new guy that is wearing the
> same
> > boots then a little collateral damage doesn't leave the server in a
> bad
> > place. HS cancellations aren't yet so precise that this matters.
> Building racy infrastructure when it can be avoided with a little care
> still seems not to be the best path to me.

Doing that will add more complexity in an area that is hard to test
effectively. I think the risk of introducing further bugs while trying
to fix this rare condition is high. Right now the conflict processing
needs more work and is often much less precise than this, so improving
this aspect of it would not be a priority. I've added it to the TODO
though. Thank you for your research.

I enclose the patch I am currently testing, as a patch-on-patch on top
of Joachim's changes, recently published on this thread. POC only as
yet.

Patch implements recovery conflict signalling using SIGUSR1
multiplexing, then uses a SessionCancelPending mode similar to Joachim's
TransactionCancelPending.

--
Simon Riggs www.2ndQuadrant.com

On Thu, Dec 31, 2009 at 6:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> Building racy infrastructure when it can be avoided with a little care
>> still seems not to be the best path to me.
>
> Doing that will add more complexity in an area that is hard to test
> effectively. I think the risk of introducing further bugs while trying
> to fix this rare condition is high. Right now the conflict processing
> needs more work and is often much less precise than this, so improving
> this aspect of it would not be a priority. I've added it to the TODO
> though. Thank you for your research.
>
> Patch implements recovery conflict signalling using SIGUSR1
> multiplexing, then uses a SessionCancelPending mode similar to Joachim's
> TransactionCancelPending.

I have reworked Simon's patch a bit and attach the result.

Quick facts:

- Hot Standby only uses SIGUSR1
- SIGINT behaves as it did before: it only cancels running statements
- pg_cancel_backend() continues to use SIGINT
- I added pg_cancel_idle_transaction() to cancel an idle transaction via
SIGUSR1
- One central function HandleCancelAction() sets the flags before calling
ProcessInterrupts(), it is called from the different signal handlers and
receives parameters about what it should do
- If a SIGUSR1 reason is used that will cancel something, ProcArrayLock is
acquired until the signal has been sent to make sure that we won't signal the
wrong backend. Does this sufficiently cover the concerns of Andres Freund
discussed upthread?

As there were so many boolean SomethingCancelPending variables I changed them
to be bitmasks and merged all of them into a single variable. However I am not
sure if we can change the name and type of especially InterruptPending that
easily as it was marked with PGDLLIMPORT...

@Simon: Is there a reason why you have not yet removed recoveryConflictMode
from PGPROC?

Joachim

On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:
> On Thu, Dec 31, 2009 at 6:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> Building racy infrastructure when it can be avoided with a little care
> >> still seems not to be the best path to me.
> >
> > Doing that will add more complexity in an area that is hard to test
> > effectively. I think the risk of introducing further bugs while trying
> > to fix this rare condition is high. Right now the conflict processing
> > needs more work and is often much less precise than this, so improving
> > this aspect of it would not be a priority. I've added it to the TODO
> > though. Thank you for your research.
> >
> > Patch implements recovery conflict signalling using SIGUSR1
> > multiplexing, then uses a SessionCancelPending mode similar to Joachim's
> > TransactionCancelPending.
>
> I have reworked Simon's patch a bit and attach the result.

Oh dear, this is exactly what I've been working on...

> Quick facts:
>
> - Hot Standby only uses SIGUSR1
> - SIGINT behaves as it did before: it only cancels running statements
> - pg_cancel_backend() continues to use SIGINT
> - I added pg_cancel_idle_transaction() to cancel an idle transaction via
> SIGUSR1
> - One central function HandleCancelAction() sets the flags before calling
> ProcessInterrupts(), it is called from the different signal handlers and
> receives parameters about what it should do
> - If a SIGUSR1 reason is used that will cancel something, ProcArrayLock is
> acquired until the signal has been sent to make sure that we won't signal the
> wrong backend. Does this sufficiently cover the concerns of Andres Freund
> discussed upthread?
>
> As there were so many boolean SomethingCancelPending variables I changed them
> to be bitmasks and merged all of them into a single variable. However I am not
> sure if we can change the name and type of especially InterruptPending that
> easily as it was marked with PGDLLIMPORT...
>
> @Simon: Is there a reason why you have not yet removed recoveryConflictMode
> from PGPROC?

--
Simon Riggs www.2ndQuadrant.com

On Thursday 07 January 2010 14:45:55 Joachim Wieland wrote:
> On Thu, Dec 31, 2009 at 6:40 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> >> Building racy infrastructure when it can be avoided with a little care
> >> still seems not to be the best path to me.
> >
> > Doing that will add more complexity in an area that is hard to test
> > effectively. I think the risk of introducing further bugs while trying
> > to fix this rare condition is high. Right now the conflict processing
> > needs more work and is often much less precise than this, so improving
> > this aspect of it would not be a priority. I've added it to the TODO
> > though. Thank you for your research.
> >
> > Patch implements recovery conflict signalling using SIGUSR1
> > multiplexing, then uses a SessionCancelPending mode similar to Joachim's
> > TransactionCancelPending.
>
> I have reworked Simon's patch a bit and attach the result.
>
> Quick facts:
>
> - Hot Standby only uses SIGUSR1
> - SIGINT behaves as it did before: it only cancels running statements
> - pg_cancel_backend() continues to use SIGINT
> - I added pg_cancel_idle_transaction() to cancel an idle transaction via
> SIGUSR1
> - One central function HandleCancelAction() sets the flags before calling
> ProcessInterrupts(), it is called from the different signal handlers and
> receives parameters about what it should do
> - If a SIGUSR1 reason is used that will cancel something, ProcArrayLock is
> acquired until the signal has been sent to make sure that we won't signal
> the wrong backend. Does this sufficiently cover the concerns of Andres
> Freund discussed upthread?
I think it solves the major concern (which I btw could easily reproduce using
software that is in production) but unfortunately not completely.
The avoided situation is:

C(Client): BEGIN; SELECT WHATEVER;
S(Standby): conflict with C
S: Starting to cancel C
C: COMMIT
S: Sending Signal to C
C: Wrong transaction is aborted

The situation not avoided is:
C: BEGIN; SELECT ...
S: conflict with C, lock procarray, sending signal(thats asynchronous), unlock
procarray
C: COMMIT; BEGIN
C: Signal arrives
C: Wrong txn is killled

It should be easy to fix this by having a cancel_localTransactionId field in the
procarray which gets cleaned uppon transaction/backend start and gets checked
in the signal handler (should be casted to sig_atomic_t)

Will cookup a patch if nobody speaks against something like that.

Andres

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> As there were so many boolean SomethingCancelPending variables I changed them
> to be bitmasks and merged all of them into a single variable.

This seems like a truly horrid idea, because those variables are set by
signal handlers. A bitmask cannot be manipulated atomically, so you
have almost certainly introduced race-condition bugs.

regards, tom lane

On Thu, Jan 7, 2010 at 4:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Joachim Wieland <joe(at)mcknight(dot)de> writes:
>> As there were so many boolean SomethingCancelPending variables I changed them
>> to be bitmasks and merged all of them into a single variable.
>
> This seems like a truly horrid idea, because those variables are set by
> signal handlers.  A bitmask cannot be manipulated atomically, so you
> have almost certainly introduced race-condition bugs.

True... However there are just a few places where the patch uses
bitmasks for modification of the variable. As Simon seems to be
working on this currently anyway, I'll leave it to him to either keep
the 5 boolean variables or do some mutual exclusion as in
HandleNotifyInterrupt() (or do something completely different).

Joachim

On Thu, Jan 7, 2010 at 3:03 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:
>> I have reworked Simon's patch a bit and attach the result.
>
> Oh dear, this is exactly what I've been working on...

Sorry, as you have posted a first patch some days ago I thought you
were waiting for feedback... Just tried to help ;-)

Joachim

On Thu, 2010-01-07 at 17:50 +0100, Joachim Wieland wrote:
> On Thu, Jan 7, 2010 at 3:03 PM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:
> >> I have reworked Simon's patch a bit and attach the result.
> >
> > Oh dear, this is exactly what I've been working on...
>
> Sorry, as you have posted a first patch some days ago I thought you
> were waiting for feedback... Just tried to help ;-)

Well, you have been very helpful, its just this last little part that is
duplicated.

--
Simon Riggs www.2ndQuadrant.com

On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:

> @Simon: Is there a reason why you have not yet removed recoveryConflictMode
> from PGPROC?

Unfortunately we still need a mechanism to mark which backends have been
cancelled already. Transaction state for virtual transactions isn't
visible on the procarray, so we need something there to indicate that a
backend has been sent a conflict. Otherwise we'd end up waiting for it
endlessly. The name will be changing though.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:
>> @Simon: Is there a reason why you have not yet removed recoveryConflictMode
>> from PGPROC?

> Unfortunately we still need a mechanism to mark which backends have been
> cancelled already. Transaction state for virtual transactions isn't
> visible on the procarray, so we need something there to indicate that a
> backend has been sent a conflict. Otherwise we'd end up waiting for it
> endlessly. The name will be changing though.

While we're discussing this: the current coding with
AbortOutOfAnyTransaction within ProcessInterrupts is *utterly* unsafe.
I realize that's just a toy placeholder, but getting rid of it has to be
on the list of stop-ship items. Right at the moment I'd prefer to see
CONFLICT_MODE_ERROR always turned into CONFLICT_MODE_FATAL than to
imagine this is going to work.

regards, tom lane

On Thu, 2010-01-07 at 12:14 -0500, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Thu, 2010-01-07 at 14:45 +0100, Joachim Wieland wrote:
> >> @Simon: Is there a reason why you have not yet removed recoveryConflictMode
> >> from PGPROC?
>
> > Unfortunately we still need a mechanism to mark which backends have been
> > cancelled already. Transaction state for virtual transactions isn't
> > visible on the procarray, so we need something there to indicate that a
> > backend has been sent a conflict. Otherwise we'd end up waiting for it
> > endlessly. The name will be changing though.
>
> While we're discussing this: the current coding with
> AbortOutOfAnyTransaction within ProcessInterrupts is *utterly* unsafe.
> I realize that's just a toy placeholder, but getting rid of it has to be
> on the list of stop-ship items. Right at the moment I'd prefer to see
> CONFLICT_MODE_ERROR always turned into CONFLICT_MODE_FATAL than to
> imagine this is going to work.

Hmmm. Can you check my current attempt? This may suffer this problem.

If, so can you explain a little more for me? Thanks.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Thu, 2010-01-07 at 12:14 -0500, Tom Lane wrote:
>> While we're discussing this: the current coding with
>> AbortOutOfAnyTransaction within ProcessInterrupts is *utterly* unsafe.
>> I realize that's just a toy placeholder, but getting rid of it has to be
>> on the list of stop-ship items. Right at the moment I'd prefer to see
>> CONFLICT_MODE_ERROR always turned into CONFLICT_MODE_FATAL than to
>> imagine this is going to work.

> Hmmm. Can you check my current attempt? This may suffer this problem.

This looks like a mess. You've duplicated a whole lot of code and not
fixed the fundamental problem.

> If, so can you explain a little more for me? Thanks.

You can not do this from inside an interrupt service routine. Period.
No amount of deck-chair-rearrangement will fix that.

As far as I can think at the moment, the best you can do is throw the
elog(ERROR), and if control gets out to the error recovery block in
PostgresMain, you can force a transaction abort there. In other words,
pretty much the same logic that was there before; the only addition that
I think is safe is to allow this to happen while DoingCommandRead, so
that you can cancel an idle transaction.

Now of course the problem with this approach, if you choose to see it as
a problem, is that somebody could trap the error and try to continue
processing. The only way you can positively guarantee that the backend
will give up whatever locks it's holding is if you elog(FATAL) instead
of trying to do normal error processing. So maybe the right thing is to
forget about CONFLICT_MODE_ERROR altogether. How critical is it that an
HS-requested query cancel be any more likely to do anything than a
regular query cancel is?

regards, tom lane

On Thursday 07 January 2010 19:12:31 Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Thu, 2010-01-07 at 12:14 -0500, Tom Lane wrote:
> >> While we're discussing this: the current coding with
> >> AbortOutOfAnyTransaction within ProcessInterrupts is *utterly* unsafe.
> >> I realize that's just a toy placeholder, but getting rid of it has to be
> >> on the list of stop-ship items. Right at the moment I'd prefer to see
> >> CONFLICT_MODE_ERROR always turned into CONFLICT_MODE_FATAL than to
> >> imagine this is going to work.
> >
> > Hmmm. Can you check my current attempt? This may suffer this problem.
>
> This looks like a mess. You've duplicated a whole lot of code and not
> fixed the fundamental problem.
>
> > If, so can you explain a little more for me? Thanks.
>
> You can not do this from inside an interrupt service routine. Period.
> No amount of deck-chair-rearrangement will fix that.
>
> As far as I can think at the moment, the best you can do is throw the
> elog(ERROR), and if control gets out to the error recovery block in
> PostgresMain, you can force a transaction abort there. In other words,
> pretty much the same logic that was there before; the only addition that
> I think is safe is to allow this to happen while DoingCommandRead, so
> that you can cancel an idle transaction.
Stupid question:

Why dont we add CHECK_FOR_INTERRUPTS (or something similar) to everything
calling recv (especially in the EINTR) case?
That way we can simply abort in the normal context without the constraint of
an interrupt handler because recv() will finish after having serviced a signal
handler.

Sure there will be some parts calling CHECK_FOR_INTERRUPTS not often enough
but thats surely not that critical - and after some time using a bit more
force is ok I guess.

Andres

On Thu, 2010-01-07 at 13:12 -0500, Tom Lane wrote:

> not fixed the fundamental problem.

(I wasn't saying that I had, only wanted to confirm the problem still
existed in the version I was currently working on.)

> How critical is it that an
> HS-requested query cancel be any more likely to do anything than a
> regular query cancel is?

Recovery needs to cancel backends in two main cases, which currently
throw CONFLICT_MODE_ERROR, though could be separated:

(1) Cancel because a snapshot might fail to see data that has now been
removed and so get an inconsistent result. We need to abort any
subtransactions that have open snapshots, which is really the whole top
level xact, unless anyone has a good plan of how to identify

(2) Cancel because a backend holds a lock on a table that has been
AccessExclusiveLocked on primary. We need to abort as far up the
transaction stack as it takes to remove the conflicting lock. We
currently abort the whole transaction.

Both of those have cases where we might need to abort further than just
the top-level subtransaction. We might be able to identify the cases
where aborting only the current subtrans is OK, and then just throw
normal ERROR for those cases. I think the cases are

* when no subxacts exist

or

* for (1): if no open portals in still active parent sub-transactions
* for (2): if any conflicting locks are held by current subxact only

--
Simon Riggs www.2ndQuadrant.com

Andres Freund <andres(at)anarazel(dot)de> writes:
> Stupid question:

> Why dont we add CHECK_FOR_INTERRUPTS (or something similar) to everything
> calling recv (especially in the EINTR) case?

We pretty much have CHECK_FOR_INTERRUPTS everywhere that it's safe
already. The problem here is not a lack of execution of
CHECK_FOR_INTERRUPTS, but what to do inside it. Although I pointed to
an interrupt service routine as being the worst case, the fact is that
Simon's code will crash the system in a large number of scenarios even
when ProcessInterrupts is called from CHECK_FOR_INTERRUPTS rather than
directly from the signal handler. You can't just abort transactions and
then return to a nest of functions that think they still have a live
transaction they can resume. This is why the standard error code path
has the AbortTransaction call out in the sigjmp catcher, not inside
elog() itself.

regards, tom lane

On Thu, 2010-01-07 at 19:23 +0100, Andres Freund wrote:
> On Thursday 07 January 2010 19:12:31 Tom Lane wrote:
> >
> > As far as I can think at the moment, the best you can do is throw the
> > elog(ERROR), and if control gets out to the error recovery block in
> > PostgresMain, you can force a transaction abort there. In other words,
> > pretty much the same logic that was there before; the only addition that
> > I think is safe is to allow this to happen while DoingCommandRead, so
> > that you can cancel an idle transaction.
> Stupid question:
>
> Why dont we add CHECK_FOR_INTERRUPTS (or something similar) to everything
> calling recv (especially in the EINTR) case?
> That way we can simply abort in the normal context without the constraint of
> an interrupt handler because recv() will finish after having serviced a signal
> handler.
>
> Sure there will be some parts calling CHECK_FOR_INTERRUPTS not often enough
> but thats surely not that critical - and after some time using a bit more
> force is ok I guess.

There's only a need for an immediate death when we were waiting for a
lock. In other cases a deferred death is possible. We could do that in
various places, such as each time we access a new data block.

--
Simon Riggs www.2ndQuadrant.com

On Thu, Jan 7, 2010 at 7:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> As far as I can think at the moment, the best you can do is throw the
> elog(ERROR), and if control gets out to the error recovery block in
> PostgresMain, you can force a transaction abort there.  In other words,
> pretty much the same logic that was there before; the only addition that
> I think is safe is to allow this to happen while DoingCommandRead, so
> that you can cancel an idle transaction.

Sorry, but to be clear about this, what do you mean with "allow this
to happen"? You mean that while DoingCommandRead it should be safe to
abort the transaction even from the signal handler or only from the
sigjmp catcher?

> Now of course the problem with this approach, if you choose to see it as
> a problem, is that somebody could trap the error and try to continue
> processing.  The only way you can positively guarantee that the backend
> will give up whatever locks it's holding is if you elog(FATAL) instead
> of trying to do normal error processing.  So maybe the right thing is to
> forget about CONFLICT_MODE_ERROR altogether.  How critical is it that an
> HS-requested query cancel be any more likely to do anything than a
> regular query cancel is?

Simon, couldn't you just translate the conflict modes to the other
cancel modes depending on DoingCommandRead (which is to be determined
from within ProcessInterrupts(), not before).

CONFLICT_MODE_ERROR && !DoingCommandRead => cancel running query
(QueryCancelPending)
CONFLICT_MODE_ERROR && DoingCommandRead => cancel idle transaction
CONFLICT_MODE_FATAL => cancel session (ProcDiePending)

Joachim

Joachim Wieland <joe(at)mcknight(dot)de> writes:
> On Thu, Jan 7, 2010 at 7:12 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> As far as I can think at the moment, the best you can do is throw the
>> elog(ERROR), and if control gets out to the error recovery block in
>> PostgresMain, you can force a transaction abort there. In other words,
>> pretty much the same logic that was there before; the only addition that
>> I think is safe is to allow this to happen while DoingCommandRead, so
>> that you can cancel an idle transaction.

> Sorry, but to be clear about this, what do you mean with "allow this
> to happen"? You mean that while DoingCommandRead it should be safe to
> abort the transaction even from the signal handler or only from the
> sigjmp catcher?

In the previous coding, nothing at all happened if DoingCommandRead.
What I am suggesting (and what should be actually safe after my fixes
a couple hours ago) is that it is okay to allow a query-cancel error
to be thrown while in DoingCommandRead state. (Of course there are
still nasty implications for the FE/BE protocol, but at least you won't
crash the backend by doing so.) What is not, and never will be, safe
is to do any sort of transaction-aborting work inside ProcessInterrupts.
You can either throw a regular elog(ERROR) and hope that subsequent
transaction abort will do what you want, or throw elog(FATAL) and let
the cleanup happen during proc_exit. The reason the second form is safe
is that control won't ever return to whatever it was you interrupted,
and so any expectations that such code might have about being able to
recover from the error and continue its processing won't matter.

regards, tom lane

On Thursday 07 January 2010 19:49:59 Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > Stupid question:
> >
> > Why dont we add CHECK_FOR_INTERRUPTS (or something similar) to everything
> > calling recv (especially in the EINTR) case?
>
> We pretty much have CHECK_FOR_INTERRUPTS everywhere that it's safe
> already. The problem here is not a lack of execution of
> CHECK_FOR_INTERRUPTS, but what to do inside it. Although I pointed to
> an interrupt service routine as being the worst case, the fact is that
> Simon's code will crash the system in a large number of scenarios even
> when ProcessInterrupts is called from CHECK_FOR_INTERRUPTS rather than
> directly from the signal handler.
I did not want to suggest using Simons code there. Sorry for the brevity.

The reason I suggested adding CHECK_FOR_INTERRUPTS into the recv code path was
that it should allow a relatively "natural" handling of canceling "IDLE IN
TRANSACTION" queries without doing anything in the interrupt handler.

I think it shouldn't be to hard to make that code path safe for
CHECK_FOR_INTERRUPTS().

I personally don't think its important to be that nice to a non-cooperating
backend (i.e. one catching our ERRORs) so I dont see any problems in just
FATAL'ing it after a couple of tries (the code retries already so...).

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> The reason I suggested adding CHECK_FOR_INTERRUPTS into the recv code path was
> that it should allow a relatively "natural" handling of canceling "IDLE IN
> TRANSACTION" queries without doing anything in the interrupt handler.

> I think it shouldn't be to hard to make that code path safe for
> CHECK_FOR_INTERRUPTS().

Idle in transaction isn't the problem (except for what it does to the
FE/BE protocol state). The problem is what happens inside a non-idle
transaction.

Since apparently I'm still not being clear enough about this, let me
spell it out:

1. Outer transaction calls, say, a plperl function.
2. plperl function executes some query via SPI, thereby starting
a subtransaction.
3. We receive an HS query-cancel interrupt. Since
!ImmediateInterruptOK, this just sets QueryCancelPending.
4. At the next occurrence of CHECK_FOR_INTERRUPTS, ProcessInterrupts
is entered.
5. According to both Simon's committed patch and his recent variant,
ProcessInterrupts executes AbortOutOfAnyTransaction and then throws
elog(ERROR).
6. plperl.c catches the elog longjmp and tries to abort its
subtransaction (loss #1), then return to the Perl interpreter
which is under no obligation to abort processing its perl script
(loss #2), and whenever it does exit, or else call SPI to try to
process another query, we're screwed because the outer transaction
is already dead (loss #3).

The situation with Perl or Python or some other PL is pretty much
the worst case, since we have no control whatever over that code
layer --- but in reality this type of scenario can play out even
without any third-party code involved. Anyplace that catches an
elog longjmp will be broken by AbortOutOfAnyTransaction inside
ProcessInterrupts, because things aren't supposed to happen in that
order.

regards, tom lane

On Thursday 07 January 2010 21:47:47 Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > The reason I suggested adding CHECK_FOR_INTERRUPTS into the recv code
> > path was that it should allow a relatively "natural" handling of
> > canceling "IDLE IN TRANSACTION" queries without doing anything in the
> > interrupt handler.
> >
> > I think it shouldn't be to hard to make that code path safe for
> > CHECK_FOR_INTERRUPTS().
>
> Idle in transaction isn't the problem (except for what it does to the
> FE/BE protocol state). The problem is what happens inside a non-idle
> transaction.
>
> Since apparently I'm still not being clear enough about this, let me
> spell it out:
> 5. According to both Simon's committed patch and his recent variant,
> ProcessInterrupts executes AbortOutOfAnyTransaction and then throws
> elog(ERROR).

> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I did not want to suggest using Simons code there. Sorry for the brevity.
should have read as "revert to old code and add two step killing (thats likely
around 10 lines or so)".

"two step killing" meaning that we signal ERROR for a few times and if nothing
happens that we like, we signal FATAL.
As the code already loops around signaling anyway that should be easy to
implement.

I have no doubt about you being right that its not practical (even more so at
this point in the release cycle) to make that AbortOutOfAnyTransaction call.

Andres

PS: Thats procarray.c: ResolveRecoveryConflictWithVirtualXIDs

Andres Freund <andres(at)anarazel(dot)de> writes:
> I did not want to suggest using Simons code there. Sorry for the brevity.
> should have read as "revert to old code and add two step killing (thats likely
> around 10 lines or so)".

> "two step killing" meaning that we signal ERROR for a few times and if nothing
> happens that we like, we signal FATAL.
> As the code already loops around signaling anyway that should be easy to
> implement.

Ah. This loop happens in the process that's trying to send the cancel
signal, correct, not the one that needs to respond to it? That sounds
fairly sane to me.

There are some things we could do to make it more likely that a cancel
of this type is accepted --- for instance, give it a distinct SQLSTATE
code that *can not* be trapped by plpgsql EXCEPTION blocks --- but there
is no practical way to guarantee it except elog(FATAL). I'm not
entirely convinced that an untrappable error would be a good thing
anyway; it's hard to argue that that's much better than a FATAL.

regards, tom lane

On Thursday 07 January 2010 22:28:46 Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > I did not want to suggest using Simons code there. Sorry for the brevity.
> > should have read as "revert to old code and add two step killing (thats
> > likely around 10 lines or so)".
> >
> > "two step killing" meaning that we signal ERROR for a few times and if
> > nothing happens that we like, we signal FATAL.
> > As the code already loops around signaling anyway that should be easy to
> > implement.
> Ah. This loop happens in the process that's trying to send the cancel
> signal, correct, not the one that needs to respond to it? That sounds
> fairly sane to me.
Yes.

> There are some things we could do to make it more likely that a cancel
> of this type is accepted --- for instance, give it a distinct SQLSTATE
> code that *can not* be trapped by plpgsql EXCEPTION blocks --- but there
> is no practical way to guarantee it except elog(FATAL). I'm not
> entirely convinced that an untrappable error would be a good thing
> anyway; it's hard to argue that that's much better than a FATAL.
Well a session which is usable after a transaction abort is quite sensible -
quite some software I know handles a failing transaction much more gracefully
than a session abort (e.g. because it has to deal with serialization failures
and such anyway).

So making it cought by fewer places and degrading to FATAL sound sensible and
relatively easy to me.
Unless somebody disagrees I will give it a shot.

@Simon: Could you possibly push your current HS state to the git repo? That
would make it easier to see whats already applied and such. That would be
really nice.

Andres

On 01/07/10 22:37, Andres Freund wrote:
> On Thursday 07 January 2010 22:28:46 Tom Lane wrote:
>> Andres Freund<andres(at)anarazel(dot)de> writes:
>>> I did not want to suggest using Simons code there. Sorry for the brevity.
>>> should have read as "revert to old code and add two step killing (thats
>>> likely around 10 lines or so)".
>>>
>>> "two step killing" meaning that we signal ERROR for a few times and if
>>> nothing happens that we like, we signal FATAL.
>>> As the code already loops around signaling anyway that should be easy to
>>> implement.
>> Ah. This loop happens in the process that's trying to send the cancel
>> signal, correct, not the one that needs to respond to it? That sounds
>> fairly sane to me.
> Yes.
>
>
>> There are some things we could do to make it more likely that a cancel
>> of this type is accepted --- for instance, give it a distinct SQLSTATE
>> code that *can not* be trapped by plpgsql EXCEPTION blocks --- but there
>> is no practical way to guarantee it except elog(FATAL). I'm not
>> entirely convinced that an untrappable error would be a good thing
>> anyway; it's hard to argue that that's much better than a FATAL.
> Well a session which is usable after a transaction abort is quite sensible -
> quite some software I know handles a failing transaction much more gracefully
> than a session abort (e.g. because it has to deal with serialization failures
> and such anyway).
>
> So making it cought by fewer places and degrading to FATAL sound sensible and
> relatively easy to me.
> Unless somebody disagrees I will give it a shot.
Ok, here is a stab at that:

1. Allow the signal multiplexing facility to transfer one sig_atomic_t
worth of data. This is usefull e.g. for making operations idempotent
or more precise.

In this the LocalBackendId is transported - that should make it
impossible to cancel the wrong transaction

Use the signal multiplexing facility.

2.

AbortTransactionAndAnySubtransactions is only used in the mainloops
error handler as it should be unproblematic there.

In the current CVS code ConditionalVirtualXactLockTableWait() in
ResolveRecoveryConflictWithVirtualXIDs does the wait for every try of
cancelling the other transaction.

I moved the retry logic into CancelVirtualTransaction(). If 50 times a
ERROR does nothing it degrades to FATAL

XXX: I temporarily do not use the skipExistingConflicts argument of
GetConflictingVirtualXIDs - I dont understand its purpose and a bit of
infrastructure is missing right now as the recoveryConflictMode is not
stored anymore anywhere. Can easily be added back though.

3.
Add a new error code ERRCODE_QUERY_CANCELED_HS for use with HS
indicating a failure that is more than a plain
ERRCODE_QUERY_CANCELED - namely it should not be caught from
various places like savepoints and in PLs.

Exemplarily I checked for that error code in plpgsql and make it
uncatcheable.

I am not sure how new errorcode codes get chosen though - and the name
is not that nice.

Opinions on that?

I copied quite a bit from Simons earlier patch...

---

Currently the patch does not yet do anything to avoid letting the
protocol out of sync. What do you think about adding a flag for error
codes not to communicate with the client (Similarly to COMERROR)?

So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or such?

Andres

PS: My current MUA suffers from a wronggone upgrade currently, so no
idea how that message will appear.

On 01/07/10 22:37, Andres Freund wrote:
> On Thursday 07 January 2010 22:28:46 Tom Lane wrote:
>> Andres Freund<andres(at)anarazel(dot)de> writes:
>>> I did not want to suggest using Simons code there. Sorry for the brevity.
>>> should have read as "revert to old code and add two step killing (thats
>>> likely around 10 lines or so)".
>>>
>>> "two step killing" meaning that we signal ERROR for a few times and if
>>> nothing happens that we like, we signal FATAL.
>>> As the code already loops around signaling anyway that should be easy to
>>> implement.
>> Ah. This loop happens in the process that's trying to send the cancel
>> signal, correct, not the one that needs to respond to it? That sounds
>> fairly sane to me.
> Yes.
>
>
>> There are some things we could do to make it more likely that a cancel
>> of this type is accepted --- for instance, give it a distinct SQLSTATE
>> code that *can not* be trapped by plpgsql EXCEPTION blocks --- but there
>> is no practical way to guarantee it except elog(FATAL). I'm not
>> entirely convinced that an untrappable error would be a good thing
>> anyway; it's hard to argue that that's much better than a FATAL.
> Well a session which is usable after a transaction abort is quite sensible -
> quite some software I know handles a failing transaction much more gracefully
> than a session abort (e.g. because it has to deal with serialization failures
> and such anyway).
>
> So making it cought by fewer places and degrading to FATAL sound sensible and
> relatively easy to me.
> Unless somebody disagrees I will give it a shot.

Alternatively the current state is available at:
http://git.postgresql.org/gitweb?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/hs-query-cancel

Its a bit raw around the edges, but I wanted to get some feedback...

Andres

On Tue, 2010-01-12 at 06:30 +0100, Andres Freund wrote:
> On 01/07/10 22:37, Andres Freund wrote:
> > On Thursday 07 January 2010 22:28:46 Tom Lane wrote:
> >> Andres Freund<andres(at)anarazel(dot)de> writes:
> >>> I did not want to suggest using Simons code there. Sorry for the brevity.
> >>> should have read as "revert to old code and add two step killing (thats
> >>> likely around 10 lines or so)".
> >>>
> >>> "two step killing" meaning that we signal ERROR for a few times and if
> >>> nothing happens that we like, we signal FATAL.
> >>> As the code already loops around signaling anyway that should be easy to
> >>> implement.
> >> Ah. This loop happens in the process that's trying to send the cancel
> >> signal, correct, not the one that needs to respond to it? That sounds
> >> fairly sane to me.
> > Yes.
> >
> >
> >> There are some things we could do to make it more likely that a cancel
> >> of this type is accepted --- for instance, give it a distinct SQLSTATE
> >> code that *can not* be trapped by plpgsql EXCEPTION blocks --- but there
> >> is no practical way to guarantee it except elog(FATAL). I'm not
> >> entirely convinced that an untrappable error would be a good thing
> >> anyway; it's hard to argue that that's much better than a FATAL.
> > Well a session which is usable after a transaction abort is quite sensible -
> > quite some software I know handles a failing transaction much more gracefully
> > than a session abort (e.g. because it has to deal with serialization failures
> > and such anyway).
> >
> > So making it cought by fewer places and degrading to FATAL sound sensible and
> > relatively easy to me.
> > Unless somebody disagrees I will give it a shot.
> Ok, here is a stab at that:
>
> 1. Allow the signal multiplexing facility to transfer one sig_atomic_t
> worth of data. This is usefull e.g. for making operations idempotent
> or more precise.
>
> In this the LocalBackendId is transported - that should make it
> impossible to cancel the wrong transaction

This doesn't remove the possibility of cancelling the wrong transaction,
it just reduces the chances. You can still cancel a session with
LocalBackendId == 1 and then have it accidentally cancel the next
session with the same backendid. That's why I didn't chase this idea
myself earlier.

Closing that loophole isn't a priority and is best left until we have
the other things have been cleaned up.

> 2.
>
> AbortTransactionAndAnySubtransactions is only used in the mainloops
> error handler as it should be unproblematic there.
>
> In the current CVS code ConditionalVirtualXactLockTableWait() in
> ResolveRecoveryConflictWithVirtualXIDs does the wait for every try of
> cancelling the other transaction.
>
> I moved the retry logic into CancelVirtualTransaction(). If 50 times a
> ERROR does nothing it degrades to FATAL

It's a good idea but not the right one, IMHO. I'd rather that the
backend decided whether the instruction results in an ERROR or a FATAL
based upon its own state, rather than have Startup process bang its head
against the wall 50 times without knowing why.

> XXX: I temporarily do not use the skipExistingConflicts argument of
> GetConflictingVirtualXIDs - I dont understand its purpose and a bit of
> infrastructure is missing right now as the recoveryConflictMode is not
> stored anymore anywhere. Can easily be added back though.

> 3.
> Add a new error code ERRCODE_QUERY_CANCELED_HS for use with HS
> indicating a failure that is more than a plain
> ERRCODE_QUERY_CANCELED - namely it should not be caught from
> various places like savepoints and in PLs.
>
> Exemplarily I checked for that error code in plpgsql and make it
> uncatcheable.
>
> I am not sure how new errorcode codes get chosen though - and the name
> is not that nice.
>
> Opinions on that?

I don't trust it yet, as a mechanism. Changing only PL/pgSQL seems
fairly broken also. Again, this seems like something that be handled
separately.

> I copied quite a bit from Simons earlier patch...

It changes a few things around and adds the ideas you've mentioned,
though seeing them as code doesn't in itself move the discussion
forwards. There are far too many new ideas in this patch for it to be
even close to acceptable, to me. Yes, lets discuss these additional
ideas, but after a basic patch has been applied.

I do value your interest and input, though racing me to rework my own
patch, then asking me to review it seems like wasted time for both of
us. I will post a new patch on ~Friday.

(Also, please make your braces { } follow the normal coding conventions
for Postgres.)

> Currently the patch does not yet do anything to avoid letting the
> protocol out of sync. What do you think about adding a flag for error
> codes not to communicate with the client (Similarly to COMERROR)?
>
> So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or such?

Seems fairly important piece.

--
Simon Riggs www.2ndQuadrant.com

On 01/12/10 09:40, Simon Riggs wrote:
> On Tue, 2010-01-12 at 06:30 +0100, Andres Freund wrote:
>> On 01/07/10 22:37, Andres Freund wrote:
>>> On Thursday 07 January 2010 22:28:46 Tom Lane wrote:
>>>> Andres Freund<andres(at)anarazel(dot)de> writes:
>>>>> I did not want to suggest using Simons code there. Sorry for the brevity.
>>>>> should have read as "revert to old code and add two step killing (thats
>>>>> likely around 10 lines or so)".
>>>>>
>>>>> "two step killing" meaning that we signal ERROR for a few times and if
>>>>> nothing happens that we like, we signal FATAL.
>>>>> As the code already loops around signaling anyway that should be easy to
>>>>> implement.
>>>> Ah. This loop happens in the process that's trying to send the cancel
>>>> signal, correct, not the one that needs to respond to it? That sounds
>>>> fairly sane to me.
>>> Yes.
>>>
>>>
>>>> There are some things we could do to make it more likely that a cancel
>>>> of this type is accepted --- for instance, give it a distinct SQLSTATE
>>>> code that *can not* be trapped by plpgsql EXCEPTION blocks --- but there
>>>> is no practical way to guarantee it except elog(FATAL). I'm not
>>>> entirely convinced that an untrappable error would be a good thing
>>>> anyway; it's hard to argue that that's much better than a FATAL.
>>> Well a session which is usable after a transaction abort is quite sensible -
>>> quite some software I know handles a failing transaction much more gracefully
>>> than a session abort (e.g. because it has to deal with serialization failures
>>> and such anyway).
>>>
>>> So making it cought by fewer places and degrading to FATAL sound sensible and
>>> relatively easy to me.
>>> Unless somebody disagrees I will give it a shot.
>> Ok, here is a stab at that:
>>
>> 1. Allow the signal multiplexing facility to transfer one sig_atomic_t
>> worth of data. This is usefull e.g. for making operations idempotent
>> or more precise.
>>
>> In this the LocalBackendId is transported - that should make it
>> impossible to cancel the wrong transaction
>
> This doesn't remove the possibility of cancelling the wrong transaction,
> it just reduces the chances. You can still cancel a session with
> LocalBackendId == 1 and then have it accidentally cancel the next
> session with the same backendid. That's why I didn't chase this idea
> myself earlier.
Hm. I don't think so. Backend Initialization clears those flags. And the
signal is sent to a specific pid so you cant send it to a new backend
when targeting the old one.

So how should that occur?

> Closing that loophole isn't a priority and is best left until we have
> the other things have been cleaned up.
Yes, maybe. It was just rather easy to fix and fixed the problem
sufficiently so that I could not reproduce it in contrast to seeing the
problem during a regular testrun.

>> 2.
>>
>> AbortTransactionAndAnySubtransactions is only used in the mainloops
>> error handler as it should be unproblematic there.
>>
>> In the current CVS code ConditionalVirtualXactLockTableWait() in
>> ResolveRecoveryConflictWithVirtualXIDs does the wait for every try of
>> cancelling the other transaction.
>>
>> I moved the retry logic into CancelVirtualTransaction(). If 50 times a
>> ERROR does nothing it degrades to FATAL
>
> It's a good idea but not the right one, IMHO. I'd rather that the
> backend decided whether the instruction results in an ERROR or a FATAL
> based upon its own state, rather than have Startup process bang its head
> against the wall 50 times without knowing why.
I don't think its that easy to keep enough state there in a safe manner.
Also the concept of retrying is not new - I just made it degrade and not
rewait for max_standby_delay (which imho is a bug).

In many cases the retries and repeated ERRORs will be enough to free the
backend out of all PG_TRY/CATCH blocks that the next ERROR reaches the
mainloop. So the retrying is quite sensible - and you cant do that part
in the signalled backend itself.

>> XXX: I temporarily do not use the skipExistingConflicts argument of
>> GetConflictingVirtualXIDs - I dont understand its purpose and a bit of
>> infrastructure is missing right now as the recoveryConflictMode is not
>> stored anymore anywhere. Can easily be added back though.
Is that a leftover from additional capabilities (deferred conflict
handling?)? Because currently there will be never a case with two
different cancellations at the same time. Also the current logic throws
away a FATAL error if a ERROR is already there.

>> 3.
>> Add a new error code ERRCODE_QUERY_CANCELED_HS for use with HS
>> indicating a failure that is more than a plain
>> ERRCODE_QUERY_CANCELED - namely it should not be caught from
>> various places like savepoints and in PLs.
>>
>> Exemplarily I checked for that error code in plpgsql and make it
>> uncatcheable.
>>
>> I am not sure how new errorcode codes get chosen though - and the name
>> is not that nice.
>>
>> Opinions on that?
>
> I don't trust it yet, as a mechanism. Changing only PL/pgSQL seems
> fairly broken also. Again, this seems like something that be handled
> separately.
Well, that was an exemplary change - its easy to fix that at other
places as well. Without any identifier of such an abort I don't see how
that could work.

We simply cant break out of nested transactions if were outside the
mainloop.

>> I copied quite a bit from Simons earlier patch...
> It changes a few things around and adds the ideas you've mentioned,
> though seeing them as code doesn't in itself move the discussion
> forwards. There are far too many new ideas in this patch for it to be
> even close to acceptable, to me. Yes, lets discuss these additional
> ideas, but after a basic patch has been applied.
Sure, the "targeted" signalling part can be left of, yes. But the rest
is mostly necessary I think.

> I do value your interest and input, though racing me to rework my own
> patch, then asking me to review it seems like wasted time for both of
> us. I will post a new patch on ~Friday.
Well. I explicitly asked whether you or somebody else is going after
this. Waited two days and only then started working on it.
And you seem to have enough on your plate...

> (Also, please make your braces { } follow the normal coding conventions
> for Postgres.)
Sure.

>> Currently the patch does not yet do anything to avoid letting the
>> protocol out of sync. What do you think about adding a flag for error
>> codes not to communicate with the client (Similarly to COMERROR)?
>>
>> So that one could do an elog(ERROR& ERROR_NO_SEND_CLIENT, .. or such?
> Seems fairly important piece.
Its quite a bit of manual work though so I wanted to be sure thats an
agreeable approach.

Andres

On Tuesday 12 January 2010 09:40:03 Simon Riggs wrote:
> On Tue, 2010-01-12 at 06:30 +0100, Andres Freund wrote:
> > Currently the patch does not yet do anything to avoid letting the
> > protocol out of sync. What do you think about adding a flag for error
> > codes not to communicate with the client (Similarly to COMERROR)?
> >
> > So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or such?
> Seems fairly important piece.
Do you aggree on the approach then? Do you want to do it?

Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> On Tuesday 12 January 2010 09:40:03 Simon Riggs wrote:
>>> So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or such?

> Do you aggree on the approach then? Do you want to do it?

Why not use the already-existing COMMERROR thing?

regards, tom lane

Hi,

The thought was that it might be helpful to be able to raise NOTICE or DEBUG* as well - but admitedly there is not a big need for it...

Andres
--
Sent from a mobile phone - please excuse brevity and formatting.
----- Ursprüngliche Mitteilung -----
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On Tuesday 12 January 2010 09:40:03 Simon Riggs wrote:
> > > > So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or such?
>
> > Do you aggree on the approach then? Do you want to do it?
>
> Why not use the already-existing COMMERROR thing?
>
>             regards, tom lane

On Tue, 2010-01-12 at 19:43 +0100, Andres Freund wrote:
> On Tuesday 12 January 2010 09:40:03 Simon Riggs wrote:
> > On Tue, 2010-01-12 at 06:30 +0100, Andres Freund wrote:
> > > Currently the patch does not yet do anything to avoid letting the
> > > protocol out of sync. What do you think about adding a flag for error
> > > codes not to communicate with the client (Similarly to COMERROR)?
> > >
> > > So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or such?
> > Seems fairly important piece.
> Do you aggree on the approach then? Do you want to do it?

If you would like to prototype something on this issue it would be
gratefully received. I will review when submitted, though I may need
other review also.

I'm still reworking other code, so things might change under you, though
not deliberately so. I will post as soon as I can, which isn't yet.

--
Simon Riggs www.2ndQuadrant.com

On Wednesday 13 January 2010 00:07:53 Simon Riggs wrote:
> On Tue, 2010-01-12 at 19:43 +0100, Andres Freund wrote:
> > On Tuesday 12 January 2010 09:40:03 Simon Riggs wrote:
> > > On Tue, 2010-01-12 at 06:30 +0100, Andres Freund wrote:
> > > > Currently the patch does not yet do anything to avoid letting the
> > > > protocol out of sync. What do you think about adding a flag for error
> > > > codes not to communicate with the client (Similarly to COMERROR)?
> > > >
> > > > So that one could do an elog(ERROR & ERROR_NO_SEND_CLIENT, .. or
> > > > such?
> > >
> > > Seems fairly important piece.
> >
> > Do you aggree on the approach then? Do you want to do it?
>
> If you would like to prototype something on this issue it would be
> gratefully received. I will review when submitted, though I may need
> other review also.
Will do - likely not before Saturday though.

> I'm still reworking other code, so things might change under you, though
> not deliberately so. I will post as soon as I can, which isn't yet.
No problem. Readapting a relatively minor amount of code isnt that hard.

Andres