On Mon, Nov 12, 2018 at 08:17:29PM -0500, Tom Lane wrote: > 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".
Okay. > 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. When first working on the patch I got to wonder if there were any intermediate states which relied on the user ID of the security context flags which could have justified its current position. Just checking now it looks safe to move up the call. I have checked as well my test cases injecting errors. What do you think about the attached? Also, I think that we should backpatch something all the way down. An ERROR in this code path is perhaps unlikely to happen but having Postgres to crash if the ERROR shows for the first query of a session is not nice. Any thoughts about that? > 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. No objections from here. I would do that only on HEAD though. -- Michael
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index a979d7e07b..d967400384 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -1809,20 +1809,38 @@ StartTransaction(void) Assert(XactTopTransactionId == InvalidTransactionId); - /* - * check the current transaction state - */ - if (s->state != TRANS_DEFAULT) - elog(WARNING, "StartTransaction while in %s state", - TransStateAsString(s->state)); + /* check the current transaction state */ + Assert(s->state == TRANS_DEFAULT); /* - * set the current transaction state information appropriately during - * start processing + * Set the current transaction state information appropriately during + * start processing. Note that once the transaction status is switched + * this process cannot fail until the user ID and the security context + * flags are fetched below. */ s->state = TRANS_START; s->transactionId = InvalidTransactionId; /* until assigned */ + /* + * initialize current transaction state fields + * + * note: prevXactReadOnly is not used at the outermost level + */ + s->nestingLevel = 1; + s->gucNestLevel = 1; + s->childXids = NULL; + s->nChildXids = 0; + s->maxChildXids = 0; + + /* + * Once the current user ID and the security context flags are fetched, + * both will be properly reset even if transaction startup fails. + */ + GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext); + + /* SecurityRestrictionContext should never be set outside a transaction */ + Assert(s->prevSecContext == 0); + /* * Make sure we've reset xact state variables * @@ -1909,20 +1927,6 @@ StartTransaction(void) /* Mark xactStopTimestamp as unset. */ xactStopTimestamp = 0; - /* - * initialize current transaction state fields - * - * note: prevXactReadOnly is not used at the outermost level - */ - s->nestingLevel = 1; - s->gucNestLevel = 1; - s->childXids = NULL; - s->nChildXids = 0; - s->maxChildXids = 0; - GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext); - /* SecurityRestrictionContext should never be set outside a transaction */ - Assert(s->prevSecContext == 0); - /* * initialize other subsystems for new transaction */
signature.asc
Description: PGP signature
