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

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Richard Guo <riguo(at)pivotal(dot)io>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
Date: 2018-10-11 03:38:10
Message-ID: 20181011033810.GB23570@paquier.xyz
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
userid-assert.patch text/x-diff 3.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2018-10-11 04:05:05 Re: Small performance tweak to run-time partition pruning
Previous Message Thomas Munro 2018-10-11 03:18:40 Re: DSM segment handle generation in background workers