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
