|From:||Michael Paquier <michael(at)paquier(dot)xyz>|
|To:||Richard Guo <riguo(at)pivotal(dot)io>|
|Subject:||Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction|
|Views:||Raw Message | Whole Thread | Download mbox|
On Wed, Oct 10, 2018 at 03:37:50PM +0800, Richard Guo wrote:
> This is a follow-up to the issue described in thread
> 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.
|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|