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

From: Richard Guo <riguo(at)pivotal(dot)io>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction
Date: 2018-10-12 06:28:36
Message-ID: CAN_9JTz2QPFwkWBm6x747Hde_rX=_hV8q6MDvuKEb+g6BVsKUg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Michael,
Thanks for your input.

On Thu, Oct 11, 2018 at 11:38 AM, Michael Paquier <michael(at)paquier(dot)xyz>
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
>

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christoph Berg 2018-10-12 07:47:51 Re: Debian mips: Failed test 'Check expected t_009_tbl data on standby'
Previous Message Ideriha, Takeshi 2018-10-12 06:19:12 Number of buckets/partitions of dshash