Re: SUBTRANS: Minimizing calls to SubTransSetParent()

On Tue, 6 Sept 2022 at 12:37, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Tue, Aug 30, 2022 at 10:16 PM Simon Riggs
> <simon(dot)riggs(at)enterprisedb(dot)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/