Hi Michael, Thanks for your input. On Thu, Oct 11, 2018 at 11:38 AM, Michael Paquier <mich...@paquier.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 >
