Re: SUBTRANS: Minimizing calls to SubTransSetParent()

On Tue, 6 Sept 2022 at 12:37, Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Aug 30, 2022 at 10:16 PM Simon Riggs
> <simon.riggs@enterprisedb.com> wrote:
>
> > PFA two patches, replacing earlier work
> > 001_new_isolation_tests_for_subxids.v3.patch
> > 002_minimize_calls_to_SubTransSetParent.v8.patch
> >
> > 001_new_isolation_tests_for_subxids.v3.patch
> > Adds new test cases to master without adding any new code, specifically
> > addressing the two areas of code that are not tested by existing tests.
> > This gives us a baseline from which we can do test driven development.
> > I'm hoping this can be reviewed and committed fairly smoothly.
> >
> > 002_minimize_calls_to_SubTransSetParent.v8.patch
> > Reduces the number of calls to subtrans below 1% for the first 64 subxids,
> > so overall will substantially reduce subtrans contention on master for the
> > typical case, as well as smoothing the overflow case.
> > Some discussion needed on this; there are various options.
> > This combines the work originally posted here with another patch posted on the
> > thread "Smoothing the subtrans performance catastrophe".
> >
> > I will do some performance testing also, but more welcome.
>
> Thanks for the updated patch, I have some questions/comments.

Thanks for the review.

> 1.
> +             * This has the downside that anyone waiting for a lock on aborted
> +             * subtransactions would not be released immediately; that may or
> +             * may not be an acceptable compromise. If not acceptable, this
> +             * simple call needs to be replaced with a loop to register the
> +             * parent for the current subxid stack, so we can walk
> back up it to
> +             * the topxid.
> +             */
> +            SubTransSetParent(subxid, GetTopTransactionId());
>
> I do not understand in which situation we will see this downside.  I
> mean if we see the logic of XactLockTableWait() then in the current
> situation also if the subtransaction is committed we directly wait on
> the top transaction by calling SubTransGetTopmostTransaction(xid);
>
> So if the lock-taking subtransaction is committed then we will wait
> directly for the top-level transaction and after that, it doesn't
> matter if we abort any of the parent subtransactions, because it will
> wait for the topmost transaction to complete.  And if the lock-taking
> subtransaction is aborted then it will anyway stop waiting because
> TransactionIdIsInProgress() should return false.

Yes, correct.

> 2.
>     /*
>      * Notice that we update pg_subtrans with the top-level xid, rather than
>      * the parent xid. This is a difference between normal processing and
>      * recovery, yet is still correct in all cases. The reason is that
>      * subtransaction commit is not marked in clog until commit processing, so
>      * all aborted subtransactions have already been clearly marked in clog.
>      * As a result we are able to refer directly to the top-level
>      * transaction's state rather than skipping through all the intermediate
>      * states in the subtransaction tree. This should be the first time we
>      * have attempted to SubTransSetParent().
>      */
>     for (i = 0; i < nsubxids; i++)
>         SubTransSetParent(subxids[i], topxid);
>
> I think this comment needs some modification because in this patch now
> in normal processing also we are setting the topxid as a parent right?

Correct

> 3.
> +    while (TransactionIdIsValid(parentXid))
> +    {
> +        previousXid = parentXid;
> +
> +        /*
> +         * Stop as soon as we are earlier than the cutoff. This saves multiple
> +         * lookups against subtrans when we have a deeply nested subxid with
> +         * a later snapshot with an xmin much higher than TransactionXmin.
> +         */
> +        if (TransactionIdPrecedes(parentXid, cutoff_xid))
> +        {
> +            *xid = previousXid;
> +            return true;
> +        }
> +        parentXid = SubTransGetParent(parentXid);
>
> Do we need this while loop if we are directly setting topxid as a
> parent, so with that, we do not need multiple iterations to go to the
> top xid?

Correct. I think we can dispense with
SubTransGetTopmostTransactionPrecedes() entirely.

I was initially trying to leave options open but that is confusing and
as a result, some parts are misleading after I merged the two patches.

I will update the patch, thanks for your scrutiny.

-- 
Simon Riggs                http://www.EnterpriseDB.com/