Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers - Mailing list pgsql-hackers

On Sat, Mar 11, 2017 at 2:10 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Mar 10, 2017 at 6:25 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Fri, Mar 10, 2017 at 11:51 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Amit Kapila <amit.kapila16@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.
>

There was a comment in existing code (proc.h) which states that it
will contain non-aborted transactions.  I agree that having it
explicitly mentioned in patch would have been much better.

/** Each backend advertises up to PGPROC_MAX_CACHED_SUBXIDS TransactionIds* for non-aborted subtransactions of its
currenttop transaction.  These* have to be treated as running XIDs by other backends.
 




> 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.
>

I agree that more analysis can help us to decide if we can use subxids
from PGPROC and if so under what conditions.  Have you considered the
another patch I have posted to fix the issue which is to do this
optimization only when subxids are not present?  In that patch, it
will remove the dependency of relying on subxids in PGPROC.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com