On Fri, Mar 10, 2017 at 6:25 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 11:51 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Amit Kapila <amit.kapil...@gmail.com> writes:
>>> Just to let you know that I think I have figured out the reason of
>>> failure. If we run the regressions with attached patch, it will make
>>> the regression tests fail consistently in same way. The patch just
>>> makes all transaction status updates to go via group clog update
>>> mechanism.
>>
>> This does *not* give me a warm fuzzy feeling that this patch was
>> ready to commit. Or even that it was tested to the claimed degree.
>>
>
> I think this is more of an implementation detail missed by me. We
> have done quite some performance/stress testing with a different
> number of savepoints, but this could have been caught only by having
> Rollback to Savepoint followed by a commit. I agree that we could
> have devised some simple way (like the one I shared above) to test the
> wide range of tests with this new mechanism earlier. This is a
> learning from here and I will try to be more cautious about such
> things in future.
After some study, I don't feel confident that it's this simple. The
underlying issue here is that TransactionGroupUpdateXidStatus thinks
it can assume that proc->clogGroupMemberXid, pgxact->nxids, and
proc->subxids.xids match the values that were passed to
TransactionIdSetPageStatus, but that's not checked anywhere. For
example, I thought about adding these assertions:
Assert(nsubxids == MyPgXact->nxids);
Assert(memcmp(subxids, MyProc->subxids.xids,
nsubxids * sizeof(TransactionId)) == 0);
There's not even a comment in the patch anywhere that notes that we're
assuming this, let alone anything that checks that it's actually true,
which seems worrying.
One thing that seems off is that we have this new field
clogGroupMemberXid, which we use to determine the XID that is being
committed, but for the subxids we think it's going to be true in every
case. Well, that seems a bit odd, right? I mean, if the contents of
the PGXACT are a valid way to figure out the subxids that we need to
worry about, then why not also it to get the toplevel XID?
Another point that's kind of bothering me is that this whole approach
now seems to me to be an abstraction violation. It relies on the set
of subxids for which we're setting status in clog matching the set of
subxids advertised in PGPROC. But actually there's a fair amount of
separation between those things. What's getting passed down to clog
is coming from xact.c's transaction state stack, which is completely
separate from the procarray. Now after going over the logic in some
detail, it does look to me that you're correct that in the case of a
toplevel commit they will always match, but in some sense that looks
accidental.
For example, look at this code from RecordTransactionAbort:
/*
* If we're aborting a subtransaction, we can immediately remove failed
* XIDs from PGPROC's cache of running child XIDs. We do that here for
* subxacts, because we already have the child XID array at hand. For
* main xacts, the equivalent happens just after this function returns.
*/
if (isSubXact)
XidCacheRemoveRunningXids(xid, nchildren, children, latestXid);
That code paints the removal of the aborted subxids from our PGPROC as
an optimization, not a requirement for correctness. And without this
patch, that's correct: the XIDs are advertised in PGPROC so that we
construct correct snapshots, but they only need to be present there
for so long as there is a possibility that those XIDs might in the
future commit. Once they've aborted, it's not *necessary* for them to
appear in PGPROC any more, but it doesn't hurt anything if they do.
However, with this patch, removing them from PGPROC becomes a hard
requirement, because otherwise the set of XIDs that are running
according to the transaction state stack and the set that are running
according to the PGPROC might be different. Yet, neither the original
patch nor your proposed fix patch updated any of the comments here.
One might wonder whether it's even wise to tie these things together
too closely. For example, you can imagine a future patch for
autonomous transactions stashing their XIDs in the subxids array.
That'd be fine for snapshot purposes, but it would break this.
Finally, I had an unexplained hang during the TAP tests while testing
out your fix patch. I haven't been able to reproduce that so it
might've just been an artifact of something stupid I did, or of some
unrelated bug, but I think it's best to back up and reconsider a bit
here.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
