Re: SUBTRANS: Minimizing calls to SubTransSetParent()

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.

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.

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?

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?

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com