Re: Is RecoveryConflictInterrupt() entirely safe in a signal handler?

On Tue, Jun 21, 2022 at 05:22:05PM +1200, Thomas Munro wrote:
> Here's one thing I got a bit confused about along the way, but it
> seems the comment was just wrong:
> 
> +                       /*
> +                        * If we can abort just the current
> subtransaction then we are OK
> +                        * to throw an ERROR to resolve the conflict.
> Otherwise drop
> +                        * through to the FATAL case.
> ...
> +                       if (!IsSubTransaction())
> ...
> +                               ereport(ERROR,
> 
> Surely this was meant to say, "If we're not in a subtransaction then
> ...", right?  Changed.

Indeed, the code does something else than what the comment says, aka
generating an ERROR if the process is not in a subtransaction,
ignoring already aborted transactions (aborted subtrans go to FATAL)
and throwing a FATAL in the other cases.  So your change looks right.

> I thought of a couple more simplifications for the big switch
> statement in ProcessRecoveryConflictInterrupt().  The special case for
> DoingCommandRead can be changed to fall through, instead of needing a
> separate ereport(FATAL).

The extra business with QueryCancelHoldoffCount and DoingCommandRead
is the only addition for the snapshot, lock and tablespace conflict
handling part.  I don't see why a reason why that could be wrong on a
close lookup.  Anyway, why don't you check QueryCancelPending on top
of QueryCancelHoldoffCount?

> Now we're down to just one ereport(FATAL), one ereport(ERROR), and a
> couple of ways to give up without erroring.  I think this makes the
> logic a lot easier to follow?

Agreed that it looks like a gain in clarity.
--
Michael