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
signature.asc
Description: PGP signature
