From 011a1f99807e54210d3bf23090ffcdee26922047 Mon Sep 17 00:00:00 2001 From: Rui Zhao Date: Wed, 5 Aug 2026 16:04:05 +0800 Subject: [PATCH 1/2] Keep heap_update()'s temporary lock out of both tuples' Xmax The Xmax computations moved after the TOAST step read the old tuple's Xmax as it stands *after* heap_update() has locked the tuple itself in order to release the buffer lock. That lock is about to be superseded by the update, and has no business being carried any further. With no concurrency at all, a TOASTing update leaves xmax = the updating transaction on the new tuple version, flagged HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY, where HEAD leaves HEAP_XMAX_INVALID; an UPDATE ... RETURNING xmax shows it. With a FOR KEY SHARE locker present, the updating xid additionally lands in the old tuple's multixact twice, once as fornokeyupd and once as nokeyupd, and the new tuple version gets a multixact where HEAD has a plain xid. Remember the Xmax and infomask the tuple had before that lock was written, and compute from those when nothing else touched the tuple meanwhile. When a locker did arrive, take our own lock back out of the Xmax instead, so that the computations see what the tuple would carry had we never locked it; whether a locker remains is then a matter of having walked the members rather than of assuming the worst. Nothing is taken back out when locking the tuple left the Xmax alone: MultiXactIdExpand() hands the existing multixact back unchanged when the same transaction is already a member with that status, and a lock it was already holding is not ours to remove. Only running lockers are carried over, the way MultiXactIdExpand() does it, since freezing of tuples whose Xmax is a multixact relies on dead members being dropped. Rebuilding the Xmax costs one extra multixact, and only when more than one locker is left behind. Also track whether the buffer lock was really released with a flag, rather than inferring it from (heaptup != newtup || newbuf != buffer): need_toast is true whenever the old tuple has an external value, so the toaster can hand back its input unchanged and the new tuple still fit on the same page, and there the lock was dropped but the inferred condition is false. Give the caller's tuple the same Xmax as the tuple that goes to the page when the toaster made a copy of it, which the computations used to do by virtue of running before the toaster. Finally, drop the mention of infomask bits computed above the Xmin/Cmin block, since nothing is computed there any more. --- src/backend/access/heap/heapam.c | 276 ++++++++++++++++++++++++++++--- 1 file changed, 251 insertions(+), 25 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 7cd5c5fe272..d68efee985f 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -95,8 +95,15 @@ static TM_Result heap_lock_updated_tuple(Relation rel, const ItemPointerData *prior_ctid, TransactionId xid, LockTupleMode mode); +static MultiXactStatus get_mxact_status_for_lock(LockTupleMode mode, + bool is_update); static void GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask, uint16 *new_infomask2); +static pg_noinline bool RemoveOwnLockFromXmax(TransactionId xmax, uint16 infomask, + TransactionId xid, MultiXactStatus status, + TransactionId *result_xmax, + uint16 *result_infomask, + uint16 *result_infomask2); static TransactionId MultiXactIdGetUpdateXid(TransactionId xmax, uint16 t_infomask); static bool DoesMultiXactIdConflict(MultiXactId multi, uint16 infomask, @@ -3308,14 +3315,21 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup, bool checked_lockers; bool locker_remains; bool id_has_external = false; + bool buffer_lock_released = false; TransactionId xmax_new_tuple, xmax_old_tuple, - xmax_lock_old_tuple = InvalidTransactionId; + xmax_lock_old_tuple = InvalidTransactionId, + xmax_before_lock, + xmax_source; uint16 infomask_old_tuple, infomask2_old_tuple, infomask_new_tuple, infomask2_new_tuple, - infomask_lock_old_tuple = 0; + infomask_lock_old_tuple = 0, + infomask_before_lock, + infomask2_before_lock, + infomask_source, + infomask2_source; Assert(ItemPointerIsValid(otid)); @@ -3752,8 +3766,7 @@ l2: /* Fill in transaction status data */ /* - * Prepare the new tuple with initial values of Xmin and Cmin, as well as - * initial infomask bits as computed above. + * Prepare the new tuple with initial values of Xmin and Cmin. * * Xmax and infomask for lockers are updated after the TOAST and/or page * extension work, as releasing the buffer lock for TOAST allows @@ -3801,6 +3814,16 @@ l2: newtupsize = MAXALIGN(newtup->t_len); + /* + * Remember the tuple's Xmax as it is now, before the block below may + * stamp a lock of our own on it. If nobody else touches the tuple while + * the buffer is unlocked, these are the values the final Xmax + * computations have to be based on. + */ + xmax_before_lock = HeapTupleHeaderGetRawXmax(oldtup.t_data); + infomask_before_lock = oldtup.t_data->t_infomask; + infomask2_before_lock = oldtup.t_data->t_infomask2; + if (need_toast || newtupsize > pagefree) { uint16 infomask2_lock_old_tuple; @@ -3905,6 +3928,7 @@ l2: unlock_vmbuffer = false; LockBuffer(buffer, BUFFER_LOCK_UNLOCK); + buffer_lock_released = true; /* * Let the toaster do its thing, if needed. @@ -3997,30 +4021,72 @@ l2: * concurrent transactions may have added tuple locks, modifying the * tuple's xmax. */ - if (heaptup != newtup || newbuf != buffer) + if (!buffer_lock_released || + (!xmax_infomask_changed(oldtup.t_data->t_infomask, + infomask_lock_old_tuple) && + TransactionIdEquals(HeapTupleHeaderGetRawXmax(oldtup.t_data), + xmax_lock_old_tuple))) + { + /* + * Either the buffer lock was never released, or the tuple's Xmax and + * infomask are still exactly the lock we wrote there ourselves. + * Either way nobody else changed the Xmax, so compute from the values + * from before that lock: it is ours, and has no business showing up + * in either tuple version's Xmax. + */ + xmax_source = xmax_before_lock; + infomask_source = infomask_before_lock; + infomask2_source = infomask2_before_lock; + } + else { + bool removed = false; + /* - * If the tuple's xmax/infomask changed from the temporary lock we - * wrote before releasing the buffer lock, a concurrent locker must - * have arrived during TOAST. We compare the tuple's current Xmax - * and infomask with the values saved before the buffer was unlocked. - * If these values have not changed, the pre-TOAST checked_lockers - * and locker_remains are still valid. If these values differ, a - * new locker has arrived: assume that the locker is still active so - * that its lock is preserved in the new tuple's Xmax. + * A locker arrived while the buffer was unlocked, so the lock we took + * has to come back out of the Xmax before the computations see it. + * + * Unless locking the tuple left the Xmax alone, that is: the same + * transaction was then already holding a lock of at least the same + * strength, MultiXactIdExpand() handed the existing multixact back + * unchanged, and what looks like our lock is one we are entitled to + * keep. */ - if (xmax_infomask_changed(oldtup.t_data->t_infomask, - infomask_lock_old_tuple) || - !TransactionIdEquals(HeapTupleHeaderGetRawXmax(oldtup.t_data), - xmax_lock_old_tuple)) + if ((infomask_before_lock & HEAP_XMAX_INVALID) || + !TransactionIdEquals(xmax_lock_old_tuple, xmax_before_lock)) + removed = RemoveOwnLockFromXmax(HeapTupleHeaderGetRawXmax(oldtup.t_data), + oldtup.t_data->t_infomask, + xid, + get_mxact_status_for_lock(*lockmode, false), + &xmax_source, &infomask_source, + &infomask2_source); + + checked_lockers = true; + if (removed) { - checked_lockers = true; + /* + * Whether any locker is left is not a matter of assumption + * anymore, since we have just been through the members. + */ + locker_remains = !(infomask_source & HEAP_XMAX_INVALID); + } + else + { + /* + * Nothing of ours to take out, or an Xmax we cannot take it out + * of. Compute from what the tuple has now and assume a locker + * remains. + */ locker_remains = true; + xmax_source = HeapTupleHeaderGetRawXmax(oldtup.t_data); + infomask_source = oldtup.t_data->t_infomask; + infomask2_source = oldtup.t_data->t_infomask2; } } - compute_new_xmax_infomask(HeapTupleHeaderGetRawXmax(oldtup.t_data), - oldtup.t_data->t_infomask, - oldtup.t_data->t_infomask2, + + compute_new_xmax_infomask(xmax_source, + infomask_source, + infomask2_source, xid, *lockmode, true, &xmax_old_tuple, &infomask_old_tuple, &infomask2_old_tuple); @@ -4032,12 +4098,12 @@ l2: * (In rare cases that might also be InvalidTransactionId and yet not have * the HEAP_XMAX_INVALID bit set; that's fine.) */ - if ((oldtup.t_data->t_infomask & HEAP_XMAX_INVALID) || - HEAP_LOCKED_UPGRADED(oldtup.t_data->t_infomask) || + if ((infomask_source & HEAP_XMAX_INVALID) || + HEAP_LOCKED_UPGRADED(infomask_source) || (checked_lockers && !locker_remains)) xmax_new_tuple = InvalidTransactionId; else - xmax_new_tuple = HeapTupleHeaderGetRawXmax(oldtup.t_data); + xmax_new_tuple = xmax_source; if (!TransactionIdIsValid(xmax_new_tuple)) { @@ -4052,7 +4118,7 @@ l2: * Note that since we're doing an update, the only possibility is that * the lockers had FOR KEY SHARE lock. */ - if (oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) + if (infomask_source & HEAP_XMAX_IS_MULTI) { GetMultiXactIdHintBits(xmax_new_tuple, &infomask_new_tuple, &infomask2_new_tuple); @@ -4073,6 +4139,21 @@ l2: heaptup->t_data->t_infomask2 |= infomask2_new_tuple; HeapTupleHeaderSetXmax(heaptup->t_data, xmax_new_tuple); + /* + * If the toaster handed back a tuple of its own, the caller's tuple has to + * be given the same values: before these computations were moved down here + * they were written to it long before the toaster ran, so callers have + * always been able to rely on the two agreeing. + */ + if (heaptup != newtup) + { + newtup->t_data->t_infomask &= ~(HEAP_XMAX_BITS); + newtup->t_data->t_infomask2 &= ~(HEAP_KEYS_UPDATED); + newtup->t_data->t_infomask |= infomask_new_tuple; + newtup->t_data->t_infomask2 |= infomask2_new_tuple; + HeapTupleHeaderSetXmax(newtup->t_data, xmax_new_tuple); + } + /* * We're about to do the actual update -- check for conflict first, to * avoid possibly having to roll back work we've just done. @@ -7770,6 +7851,151 @@ GetMultiXactIdHintBits(MultiXactId multi, uint16 *new_infomask, *new_infomask2 = bits2; } +/* + * RemoveOwnLockFromXmax + * Take the lock we placed on a tuple ourselves back out of its Xmax. + * + * heap_update() locks the tuple it is about to update before it releases the + * buffer lock for the TOAST and/or page extension work, so that concurrent + * updates and deletes stay away while the buffer is unlocked. Lockers that + * do not conflict with that lock are free to join the Xmax in the meantime, + * and once the buffer lock is back the Xmax computations have to account for + * them. They must not, however, account for our own lock: it is about to be + * superseded by the update itself, and carrying it any further would leave + * the updating transaction listed as a locker of the new tuple version. + * + * Given the Xmax of such a tuple, this hands back the Xmax it would have if + * our lock had never been there, in the representation a tuple header uses. + * Only a single occurrence of (xid, status) is removed, so a lock the same + * transaction was already holding under a different status is preserved; the + * caller is responsible for not calling this at all when locking the tuple + * did not add anything in the first place. + * + * Returns false, leaving the output parameters alone, if the Xmax is not in a + * shape this can be done for; the caller then has to make do with the Xmax as + * it stands. + */ +static pg_noinline bool +RemoveOwnLockFromXmax(TransactionId xmax, uint16 infomask, + TransactionId xid, MultiXactStatus status, + TransactionId *result_xmax, uint16 *result_infomask, + uint16 *result_infomask2) +{ + MultiXactMember *members; + MultiXactMember *remaining; + int nmembers; + int nremaining = 0; + bool found = false; + + /* + * Somebody joining the lock we took turns the Xmax into a multixact, so + * anything else means the tuple did not change the way we assumed. + */ + if (!(infomask & HEAP_XMAX_IS_MULTI) || HEAP_LOCKED_UPGRADED(infomask)) + return false; + + nmembers = GetMultiXactIdMembers((MultiXactId) xmax, &members, false, + HEAP_XMAX_IS_LOCKED_ONLY(infomask)); + if (nmembers <= 0) + return false; + + remaining = palloc(sizeof(MultiXactMember) * nmembers); + + for (int i = 0; i < nmembers; i++) + { + if (!found && + TransactionIdEquals(members[i].xid, xid) && + members[i].status == status) + { + found = true; + continue; + } + + /* + * An update of this tuple by anyone else would have made us wait long + * before this point, so there should be no updater among the members. + * Give up rather than guess if one turns up anyway. + */ + if (ISUPDATE_from_mxstatus(members[i].status)) + { + pfree(remaining); + pfree(members); + return false; + } + + /* + * Keep only the lockers that are still running, the way + * MultiXactIdExpand() does: dropping the dead ones is what freezing + * of tuples whose Xmax is a multixact relies on. + */ + if (!TransactionIdIsInProgress(members[i].xid)) + continue; + + remaining[nremaining++] = members[i]; + } + + pfree(members); + + if (!found) + { + pfree(remaining); + return false; + } + + if (nremaining == 0) + { + /* our lock was the only one, so the tuple is back to having no Xmax */ + *result_xmax = InvalidTransactionId; + *result_infomask = HEAP_XMAX_INVALID; + *result_infomask2 = 0; + } + else if (nremaining == 1) + { + /* + * A lone locker lives in the tuple header directly, the way + * heap_lock_tuple() would have left it, rather than as a multixact. + * Only a key share locker can have joined while we held the tuple + * locked, so that is the status to expect here; the rest are for + * completeness. + */ + *result_xmax = remaining[0].xid; + *result_infomask = HEAP_XMAX_LOCK_ONLY; + *result_infomask2 = 0; + + switch (remaining[0].status) + { + case MultiXactStatusForKeyShare: + *result_infomask |= HEAP_XMAX_KEYSHR_LOCK; + break; + case MultiXactStatusForShare: + *result_infomask |= HEAP_XMAX_SHR_LOCK; + break; + case MultiXactStatusForNoKeyUpdate: + *result_infomask |= HEAP_XMAX_EXCL_LOCK; + break; + case MultiXactStatusForUpdate: + *result_infomask |= HEAP_XMAX_EXCL_LOCK; + *result_infomask2 |= HEAP_KEYS_UPDATED; + break; + default: + /* updaters were rejected above */ + elog(ERROR, "unexpected multixact status %d", + (int) remaining[0].status); + } + } + else + { + MultiXactId multi; + + multi = MultiXactIdCreateFromMembers(nremaining, remaining); + *result_xmax = (TransactionId) multi; + GetMultiXactIdHintBits(multi, result_infomask, result_infomask2); + } + + pfree(remaining); + return true; +} + /* * MultiXactIdGetUpdateXid * -- 2.43.7