Re: Restore CurrentUserId only if 'prevUser' is valid when aborttransaction

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