Re: Hot Standby and query cancel

We've been chewing around query cancel on Hot Standby and I think things
have got fairly confusing, hence a new thread.

I enclose a patch that includes all the things that we all agree on so
far, in my understanding

* Recovery conflict processing uses SIGUSR1 rather than shmem per Tom,
while holding ProcArrayLock per Andres

* CONFLICT_MODE_ERROR throws ERROR when in a transaction, not idle and
not in subtransaction, otherwise becomes CONFLICT_MODE_FATAL per Tom and
other discussion

* Recovery abort message has additional detail, per Heikki

It doesn't include anything still under discussion, though is intended
as a base upon which further patches can progress independently.

* Infrastructure for supercancel, by Joachim Wieland

* Any of the many further ideas by Andres Freund

Please review this so we can move onto taking other issues one by one.
This is also a base for other HS work I need to complete.

I am still testing patch, so should be confident to commit tomorrow
barring issues.

--
Simon Riggs www.2ndQuadrant.com

Hi Simon,

On Wednesday 13 January 2010 19:24:22 Simon Riggs wrote:
> We've been chewing around query cancel on Hot Standby and I think things
> have got fairly confusing, hence a new thread.
Good idea.

> I enclose a patch that includes all the things that we all agree on so
> far, in my understanding
cool.

> * Recovery conflict processing uses SIGUSR1 rather than shmem per Tom,
> while holding ProcArrayLock per Andres
>
> * CONFLICT_MODE_ERROR throws ERROR when in a transaction, not idle and
> not in subtransaction, otherwise becomes CONFLICT_MODE_FATAL per Tom and
> other discussion
>
> * Recovery abort message has additional detail, per Heikki
>
> It doesn't include anything still under discussion, though is intended
> as a base upon which further patches can progress independently.

> I am still testing patch, so should be confident to commit tomorrow
> barring issues.
I have only looked at briefly because right now I dont have the time (going to
eat at a friends place...) but I think I spotted an issue:
The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt is not
correct right now because that returns true for TBLOCK_SUBABORT as well.
Wouldnt that mess with the case where were in a failed subxact and then
rollback only that subxact?

Andres

On Wed, 2010-01-13 at 19:58 +0100, Andres Freund wrote:

> > I am still testing patch, so should be confident to commit tomorrow
> > barring issues.
> I have only looked at briefly because right now I dont have the time (going to
> eat at a friends place...) but I think I spotted an issue:
> The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt is not
> correct right now because that returns true for TBLOCK_SUBABORT as well.
> Wouldnt that mess with the case where were in a failed subxact and then
> rollback only that subxact?

Well spotted, yes.

--
Simon Riggs www.2ndQuadrant.com

On Wed, 2010-01-13 at 19:23 +0000, Simon Riggs wrote:
> On Wed, 2010-01-13 at 19:58 +0100, Andres Freund wrote:
>
> > > I am still testing patch, so should be confident to commit tomorrow
> > > barring issues.
> > I have only looked at briefly because right now I dont have the time (going to
> > eat at a friends place...) but I think I spotted an issue:
> > The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt is not
> > correct right now because that returns true for TBLOCK_SUBABORT as well.
> > Wouldnt that mess with the case where were in a failed subxact and then
> > rollback only that subxact?
>
> Well spotted, yes.

Latest version of same patch, but uses conflict reasons passed-thru
directly from recovery to backend.

Please review, no commit before tomorrow.

--
Simon Riggs www.2ndQuadrant.com

On Thursday 14 January 2010 13:21:07 Simon Riggs wrote:
> On Wed, 2010-01-13 at 19:23 +0000, Simon Riggs wrote:
> > On Wed, 2010-01-13 at 19:58 +0100, Andres Freund wrote:
> > > > I am still testing patch, so should be confident to commit tomorrow
> > > > barring issues.
> > >
> > > I have only looked at briefly because right now I dont have the time
> > > (going to eat at a friends place...) but I think I spotted an issue:
> > > The IsAbortedTransactionBlockState() check in RecoveryConflictInterrupt
> > > is not correct right now because that returns true for TBLOCK_SUBABORT
> > > as well. Wouldnt that mess with the case where were in a failed
> > > subxact and then rollback only that subxact?
> >
> > Well spotted, yes.
>
> Latest version of same patch, but uses conflict reasons passed-thru
> directly from recovery to backend.
>
> Please review, no commit before tomorrow.
I only noted a tiny thing (which was present earlier on):

snprintf(waitactivitymsg, sizeof(waitactivitymsg),
"waiting for max_standby_delay (%u ms)",
MaxStandbyDelay);
in ResolveRecoveryConflictWithVirtualXIDs.

Shouldnt that be seconds? Otherwise the check in WaitExceedsMaxStandbyDelay is
wrong...

Andres