commit a3705442ab0c8f66f600add6181505dac6c1ebf4 Author: Simon Riggs Date: Tue Sep 13 11:32:32 2022 +0100 Streamline patches and comments diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c index 922621b4fc..140ad78530 100644 --- a/src/backend/access/transam/subtrans.c +++ b/src/backend/access/transam/subtrans.c @@ -137,14 +137,8 @@ SubTransGetParent(TransactionId xid) /* * SubTransGetTopmostTransaction * - * Returns the topmost transaction of the given transaction id. - * - * Because we cannot look back further than TransactionXmin, it is possible - * that this function will lie and return an intermediate subtransaction ID - * instead of the true topmost parent ID. This is OK, because in practice - * we only care about detecting whether the topmost parent is still running - * or is part of a current snapshot's list of still-running transactions. - * Therefore, any XID before TransactionXmin is as good as any other. + * Returns the topmost transaction of the given transaction id, if one has + * been recorded in pg_subtrans. */ TransactionId SubTransGetTopmostTransaction(TransactionId xid) @@ -177,62 +171,6 @@ SubTransGetTopmostTransaction(TransactionId xid) return previousXid; } -/* - * SubTransGetTopmostTransactionPrecedes - * - * Different form of SubTransGetTopmostTransaction() that minimizes the number - * of iterations required to get the parent or stop if it is earlier than cutoff. - */ -bool -SubTransGetTopmostTransactionPrecedes(TransactionId *xid, TransactionId cutoff_xid, bool standby) -{ - TransactionId parentXid = *xid, - previousXid = *xid; - - /* Can't ask about stuff that might not be around anymore */ - Assert(TransactionIdFollowsOrEquals(cutoff_xid, TransactionXmin)); - Assert(TransactionIdFollowsOrEquals(*xid, cutoff_xid)); - - 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); - - /* - * By convention the parent xid gets allocated first, so should always - * precede the child xid. Anything else points to a corrupted data - * structure that could lead to an infinite loop, so exit. - */ - if (!TransactionIdPrecedes(parentXid, previousXid)) - elog(ERROR, "pg_subtrans contains invalid entry: xid %u points to parent xid %u", - previousXid, parentXid); - - /* - * subxids always point directly at parent on standby, so we can avoid - * multiple loops in that case. - */ - if (standby) - break; - } - - Assert(TransactionIdIsValid(previousXid)); - - *xid = previousXid; - - return false; -} - /* * Initialization of shared memory for SUBTRANS */ diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c index 5577e5b8df..04e597b5ec 100644 --- a/src/backend/access/transam/xact.c +++ b/src/backend/access/transam/xact.c @@ -713,7 +713,7 @@ AssignTransactionId(TransactionState s) * 2. When IsolationIsSerializable() we sometimes need to access topxid * This occurs only when SERIALIZABLE is requested by app user. * - * 3. When TransactionIdSetStatus will use a status of SUB_COMMITTED, + * 3. When TransactionIdSetTreeStatus will use a status of SUB_COMMITTED, * which then requires us to consult subtrans to find parent, which * is needed to avoid race condition. In this case we ask Clog/Xact * module if TransactionIdsAreOnSameXactPage(). Since we start a new @@ -726,14 +726,14 @@ AssignTransactionId(TransactionState s) /* * Insert entries into subtrans for this xid, noting that the entry * points directly to the topxid, not the immediate parent. This is - * done for two reasons, (1) so it is faster in a long chain of subxids + * done for two reasons: + * (1) so it is faster in a long chain of subxids, because the + * algorithm is then O(1), no matter how many subxids are assigned. * (2) so that we don't need to set subxids for unregistered parents. - * 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. + * Previously when we set the parent to be the immediate parent, + * we then had to set the parent in all cases to maintain the chain + * of values to reach the topxid. If all subxids point to topxid, + * then they are independent of each other and we can skip some. */ SubTransSetParent(subxid, GetTopTransactionId()); } diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index 0067fa327f..dae3832e87 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -1316,14 +1316,14 @@ ProcArrayApplyXidAssignment(TransactionId topxid, /* * 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 + * the parent xid, which is 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(). + * have attempted to SubTransSetParent() for this xid in recovery. + * XXX this may be optimized later, but we don't have good test coverage. */ for (i = 0; i < nsubxids; i++) SubTransSetParent(subxids[i], topxid); diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index ae9dc0dd92..e50a640231 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -2310,13 +2310,13 @@ retry_search: { if (pg_lfind32(xid, snapshot->xip, snapshot->xcnt)) return true; - } - /* - * If we have the parent xid, then the xid is not in snapshot - */ - if (have_parent) - return false; + /* + * If we have the parent xid, then the xid is not in snapshot + */ + if (have_parent) + return false; + } /* * Now search subxip, which contains first 64 subxids of each topxid. @@ -2326,6 +2326,8 @@ retry_search: if (!have_parent && snapshot->suboverflowed) { + TransactionId pxid; + /* TESTED-BY src/test/isolation/specs/subx-overflow.spec test1 and test2 */ /* @@ -2335,26 +2337,34 @@ retry_search: * * It is important we do this step last because it is expensive, * and if everybody does this then SubTransSLRU glows white hot. - * - * Use SubTransGetTopmostTransactionPrecedes(), which has been - * specially provided to help here. This does two things for us: - * - * 1. On standby, get the parent directly, since in a standby SUBTRANS - * always stores the direct parent only. Doing this avoids - * one lookup of subtrans, since SubTransGetTopmostTransaction() - * always does at least 2 SUBTRANS lookups, the last lookup is - * how the loop knows it has found the parent in normal running. - * - * 2. Stops the iteration to find the parent as soon as we find an - * xid earlier than snapshot->xmin, so we do the minimum lookups. */ - if (SubTransGetTopmostTransactionPrecedes(&xid, - snapshot->xmin, - snapshot->takenDuringRecovery)) + + /* + * Snapshot overflowed, so convert xid to top-level. This is safe + * because we eliminated too-old XIDs above. Every overflowed subxid + * will always have a parent recorded during AssignTransactionId() + * so this should always return a valid TransactionId, if a subxact. + */ + pxid = SubTransGetTopmostTransaction(xid); + + /* + * If xid was indeed a subxact, we might now have an xid < xmin, + * so recheck to avoid an array scan. No point in rechecking + * xmax. If it wasn't a subxact, pxid will be invalid, so this test + * will do the right thing also. + */ + if (TransactionIdPrecedes(pxid, snapshot->xmin)) return false; - have_parent = true; - goto retry_search; /* search arrays again, now we have parent */ + /* + * Check whether xid was a subxact. If we now have a parent xid, loop. + */ + if (TransactionIdPrecedes(pxid, xid)) + { + xid = pxid; + have_parent = true; + goto retry_search; /* search arrays again, now we have parent */ + } } return false; diff --git a/src/include/access/subtrans.h b/src/include/access/subtrans.h index fc4103b769..f94e116640 100644 --- a/src/include/access/subtrans.h +++ b/src/include/access/subtrans.h @@ -17,7 +17,6 @@ extern void SubTransSetParent(TransactionId xid, TransactionId parent); extern TransactionId SubTransGetParent(TransactionId xid); extern TransactionId SubTransGetTopmostTransaction(TransactionId xid); -extern bool SubTransGetTopmostTransactionPrecedes(TransactionId *xid, TransactionId cutoff_xid, bool standby); extern Size SUBTRANSShmemSize(void); extern void SUBTRANSShmemInit(void);