Restore CurrentUserId only if 'prevUser' is valid when abort transaction

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Restore CurrentUserId only if 'prevUser' is valid when abort transaction

Richard Guo-2
Hi,

This is a follow-up to the issue described in thread

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 (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction

Michael Paquier-2
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

userid-assert.patch (3K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction

Richard Guo-2
Hi Michael,
Thanks for your input.

On Thu, Oct 11, 2018 at 11:38 AM, Michael Paquier <[hidden email]> 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