Michael Paquier <mich...@paquier.xyz> writes:
> So, I have spent a couple of hours today looking a bit more at the
> problem, and I have hacked the attached patch that I am pretty happy
> with:
I don't like this too much, because it does not scale: there can be
only one action that can rely on executing "just before we switch to
TRANS_INPROGRESS".
I think the real bug here is that a bunch of potentially-failable
operations have been thrown in before we've finished initializing the
TransactionState to minimal sanity. (Inserting code with the aid of a
dartboard seems to be a chronic disease around here :-(.) Since
GetUserIdAndSecContext is *not* an operation that can fail, there's
no reason why we need to expend a lot of effort on the possibility that
it hasn't happened. What we ought to do is move that and the rest of the
"initialize current transaction state fields" stanza up to before we start
doing things like calling RecoveryInProgress(). And put in a comment to
clearly mark where we first allow failure to occur.
I'd be strongly inclined to change the elog(WARNING) at line 1815
to an assertion, because calling elog exposes us to all kinds of
hazards that we don't need here.
regards, tom lane
