On Tue, Oct 23, 2018 at 02:40:30PM +0900, Michael Paquier wrote: > Actually, as StartSubTransaction also switches to TRANS_START for a > savepoint, if there is an error until the state is switched to > TRANS_INPROGRESS then the code would fail to switch back to > CurrentUserId even if it is set, and it should be switched. So that > solution is not correct either as AtSubStart_ResourceOwner() or such > could fail on memory allocation. That's unlikely going to happen, but > it could.
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: - Normal transactions can rely on TRANS_START to decide if the security context can be reset or not. - When defining a savepoint, the subtransaction state is initialized by PushTransaction() before being switched to its sub-in-progress state when the query which created the savepoint commits. In this case, we should call GetUserIdAndSecContext() just before switching the transaction context. The patch includes a set tweaks I used to inject some errors in specific code paths and trigger failures, checking if a security context which has been set is correctly reset: - /tmp/error_start for the end of StartTransaction - /tmp/error_sub for the end of StartSubTransaction - /tmp/error_push for the end of PushTransaction. Like on HEAD, this patch still triggers the following WARNING if injecting an error in PushTransaction as StartSubTransaction has not switched the status of the transaction yet: AbortSubTransaction while in DEFAULT state Another WARNING which can be reached is the following if injecting an error in StartSubTransaction: AbortSubTransaction while in START state Per the set of routines called when starting the subtransaction, I think that we ought to do as main transactions and silence this warning equally. I am attaching the patch for review by others. Please note that this includes the error injections to ease tests. -- Michael
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 8c1621d949..92913fbb94 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1823,6 +1823,12 @@ StartTransaction(void)
s->state = TRANS_START;
s->transactionId = InvalidTransactionId; /* until assigned */
+ {
+ struct stat statbuf;
+ if (stat("/tmp/error_start", &statbuf) == 0)
+ elog(ERROR, "error injected in StartTransaction");
+ }
+
/*
* Make sure we've reset xact state variables
*
@@ -1919,9 +1925,6 @@ StartTransaction(void)
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
@@ -1930,6 +1933,15 @@ StartTransaction(void)
AtStart_Cache();
AfterTriggerBeginXact();
+ /*
+ * Security context initialization needs to happen just before switching
+ * the transaction state as resetting it to its previous state depends
+ * on the transaction status being TRANS_START.
+ */
+ GetUserIdAndSecContext(&s->prevUser, &s->prevSecContext);
+ /* SecurityRestrictionContext should never be set outside a transaction */
+ Assert(s->prevSecContext == 0);
+
/*
* done with start processing, set current transaction state to "in
* progress"
@@ -2520,28 +2532,34 @@ AbortTransaction(void)
* check the current transaction state
*/
is_parallel_worker = (s->blockState == TBLOCK_PARALLEL_INPROGRESS);
- if (s->state != TRANS_INPROGRESS && s->state != TRANS_PREPARE)
+ if (s->state != TRANS_START &&
+ s->state != TRANS_INPROGRESS &&
+ s->state != TRANS_PREPARE)
elog(WARNING, "AbortTransaction while in %s state",
TransStateAsString(s->state));
Assert(s->parent == NULL);
- /*
- * set the current transaction state information appropriately during the
- * abort processing
- */
- s->state = TRANS_ABORT;
-
/*
* Reset user ID which might have been changed transiently. We need this
* to clean up in case control escaped out of a SECURITY DEFINER function
* or other local change of CurrentUserId; therefore, the prior value of
* SecurityRestrictionContext also needs to be restored.
*
+ * If the transaction is aborted when it has been partially started, then
+ * the user ID does not need to be set to its previous value.
+ *
* (Note: it is not necessary to restore session authorization or role
* settings here because those can only be changed via GUC, and GUC will
* take care of rolling them back if need be.)
*/
- SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
+ if (s->state != TRANS_START)
+ SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
+
+ /*
+ * set the current transaction state information appropriately during the
+ * abort processing
+ */
+ s->state = TRANS_ABORT;
/* If in parallel mode, clean up workers and exit parallel mode. */
if (IsInParallelMode())
@@ -3013,15 +3031,6 @@ AbortCurrentTransaction(void)
}
else
{
- /*
- * We can get here after an error during transaction start
- * (state will be TRANS_START). Need to clean up the
- * incompletely started transaction. First, adjust the
- * low-level state to suppress warning message from
- * AbortTransaction.
- */
- if (s->state == TRANS_START)
- s->state = TRANS_INPROGRESS;
AbortTransaction();
CleanupTransaction();
}
@@ -4355,15 +4364,6 @@ AbortOutOfAnyTransaction(void)
}
else
{
- /*
- * We can get here after an error during transaction start
- * (state will be TRANS_START). Need to clean up the
- * incompletely started transaction. First, adjust the
- * low-level state to suppress warning message from
- * AbortTransaction.
- */
- if (s->state == TRANS_START)
- s->state = TRANS_INPROGRESS;
AbortTransaction();
CleanupTransaction();
}
@@ -4544,6 +4544,12 @@ StartSubTransaction(void)
s->state = TRANS_START;
+ {
+ struct stat statbuf;
+ if (stat("/tmp/error_sub", &statbuf) == 0)
+ elog(ERROR, "error injected in StartSubTransaction");
+ }
+
/*
* Initialize subsystems for new subtransaction
*
@@ -4741,7 +4747,8 @@ AbortSubTransaction(void)
*/
ShowTransactionState("AbortSubTransaction");
- if (s->state != TRANS_INPROGRESS)
+ if (s->state != TRANS_START &&
+ s->state != TRANS_INPROGRESS)
elog(WARNING, "AbortSubTransaction while in %s state",
TransStateAsString(s->state));
@@ -4750,6 +4757,9 @@ AbortSubTransaction(void)
/*
* Reset user ID which might have been changed transiently. (See notes in
* AbortTransaction.)
+ *
+ * Note that for a subtransaction DefineSavepoint() has already called
+ * PushTransaction(), which has set GetUserIdAndSecContext().
*/
SetUserIdAndSecContext(s->prevUser, s->prevSecContext);
@@ -4912,6 +4922,12 @@ PushTransaction(void)
* assume that it necessarily has a transaction context, resource owner,
* or XID.
*/
+
+ {
+ struct stat statbuf;
+ if (stat("/tmp/error_push", &statbuf) == 0)
+ elog(ERROR, "error injected in PushTransaction");
+ }
}
/*
signature.asc
Description: PGP signature
