On Thu, Oct 11, 2018 at 11:38 AM, Michael Paquier <michael@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.
