On Wed, Oct 10, 2018 at 03:37:50PM +0800, Richard Guo wrote: > This is a follow-up to the issue described in thread > https://www.postgresql.org/message-id/CAMbWs4-Mys%3DhBQSevTA8Zpd-TYFnb%3DXuHhN2TnktXMsfMUbjiQ%40mail.gmail.com > > In short, during the first transaction starting phase within a backend, if > there is an 'ereport' after setting transaction state but before saving > CurrentUserId into 'prevUser' in TransactionStateData, 'prevUser' will > remain as InvalidOid. Then in AbortTransaction(), CurrentUserId is > restored with 'prevUser'. As a result, CurrentUserId will be > InvalidOid in the rest of the session. > > Attached is a patch that fixes this issue.
I guess that's an issue showing up with Greenplum as you folks are
likely tweaking how a transaction start happens?
It is as easy as doing something like that in StartTransaction() to get
into a failed state:
s->state = TRANS_START;
s->transactionId = InvalidTransactionId; /* until assigned */
+ {
+ struct stat statbuf;
+ if (stat("/tmp/hoge", &statbuf) == 0)
+ elog(ERROR, "hoge invalid state!");
+ }
Then do something like the following:
1) Start a session
2) touch /tmp/hoge
3) Issue BEGIN, which fails and initializes CurrentUserId to InvalidOid.
4) rm /tmp/hoge
3) any DDL causes the system to crash.
Anyway, looking at the patch, I am poked by the comment on top of
GetUserIdAndSecContext which states that InvalidOid can be a possible
value. It seems to me that the root of the problem is that TRANS_STATE
is enforced to TRANS_INPROGRESS when aborting a transaction in a
starting state, in which case we should not have to reset CurrentUserId
as it has never been set. The main reason why this was done is to
prevent a warning message to show up.
Tom, eedb068c0 is in cause here, and that's your commit. Could you
check if something like the attached is adapted? I am pretty sure that
we still want the sub-transaction part to still reset CurrentUserId
unconditionally by the way.
Thanks,
--
Michael
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 6cd00d9aaa..4dbedc3e00 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -1924,7 +1924,12 @@ StartTransaction(void)
Assert(s->prevSecContext == 0);
/*
- * initialize other subsystems for new transaction
+ * Initialize other subsystems for new transaction.
+ *
+ * XXX: Those had better be designed to never issue an ERROR as
+ * GetUserIdAndSecContext() has been called so as a transaction
+ * still marked as starting is able to reset what has been set
+ * by the previous call!
*/
AtStart_GUC();
AtStart_Cache();
@@ -2520,28 +2525,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 +3024,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 +4357,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();
}
signature.asc
Description: PGP signature
