From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Richard Guo <riguo(at)pivotal(dot)io> |
Cc: | pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: Restore CurrentUserId only if 'prevUser' is valid when abort transaction |
Date: | 2018-10-11 03:38:10 |
Message-ID: | 20181011033810.GB23570@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
userid-assert.patch | text/x-diff | 3.2 KB |
From | Date | Subject | |
---|---|---|---|
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 |