Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
On Tue, Nov 13, 2018 at 04:08:04PM -0500, Tom Lane wrote: > Well, if it is an issue then it's an issue for back branches too. > It's fine, I think, as long as the warning stays a warning ... > but there are lots of ways in which trying to print a warning > might turn into an error. At the end I have agreed with this position, and patched back-branches so as they use an assertion instead of the elog(). So committed. -- Michael signature.asc Description: PGP signature
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
Michael Paquier writes: > On Mon, Nov 12, 2018 at 08:17:29PM -0500, Tom Lane wrote: >> 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 :-(.) > 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? Looks sane to me. > Also, I think that we should backpatch something all the way down. Yes. >> 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. Well, if it is an issue then it's an issue for back branches too. It's fine, I think, as long as the warning stays a warning ... but there are lots of ways in which trying to print a warning might turn into an error. regards, tom lane
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
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
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
Michael Paquier 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
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
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
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
On Fri, Oct 12, 2018 at 02:28:36PM +0800, Richard Guo wrote: > I think it is a better idea to avoid adjusting the state to TRANS_INPROGRESS > from TRANS_START when aborting a transaction, as your patch does, since its > only purpose is to suppress warning message. 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. -- Michael signature.asc Description: PGP signature
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
Hi Michael,
Thanks for your input.
On Thu, Oct 11, 2018 at 11:38 AM, Michael Paquier
wrote:
> 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.
>
Yes, you're right. This issue shows up when we were adding
inside StartTransaction()
some resource-group related logic which would error out in some cases.
Your example reproduces the same scene.
>
> 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.
>
>From the comment, Get/SetUserIdAndSecContext() has considered the case of
InvalidOid, but fails to handle it properly in AbortTransaction().
I think it is a better idea to avoid adjusting the state to TRANS_INPROGRESS
from TRANS_START when aborting a transaction, as your patch does, since its
only purpose is to suppress warning message.
>
> 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
>
Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
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 clea
Restore CurrentUserId only if 'prevUser' is valid when abort transaction
Hi, 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. Thanks Richard 0001-Restore-CurrentUserId-only-if-prevUser-is-valid.patch Description: Binary data
