diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c index 075d78131a0..ee5b241c928 100644 --- a/contrib/pgrowlocks/pgrowlocks.c +++ b/contrib/pgrowlocks/pgrowlocks.c @@ -165,8 +165,7 @@ pgrowlocks(PG_FUNCTION_ARGS) values[Atnum_ismulti] = pstrdup("true"); - allow_old = !(infomask & HEAP_LOCK_MASK) && - (infomask & HEAP_XMAX_LOCK_ONLY); + allow_old = HEAP_LOCKED_UPGRADED(infomask); nmembers = GetMultiXactIdMembers(xmax, &members, allow_old); if (nmembers == -1) { diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 1f1bac5c6be..4822fbbe47e 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3359,6 +3359,7 @@ l2: * 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) || (checked_lockers && !locker_remains)) xmax_new_tuple = InvalidTransactionId; else @@ -4641,8 +4642,7 @@ l5: * pg_upgrade; both MultiXactIdIsRunning and MultiXactIdExpand assume * that such multis are never passed. */ - if (!(old_infomask & HEAP_LOCK_MASK) && - HEAP_XMAX_IS_LOCKED_ONLY(old_infomask)) + if (HEAP_LOCKED_UPGRADED(old_infomask)) { old_infomask &= ~HEAP_XMAX_IS_MULTI; old_infomask |= HEAP_XMAX_INVALID; @@ -5001,6 +5001,16 @@ l4: int i; MultiXactMember *members; + /* + * We don't need a test for pg_upgrade'd tuples: this is only + * applied to tuples after the first in an update chain. Said + * first tuple in the chain may well be locked-in-9.2-and- + * pg_upgraded, but that one was already locked by our caller, + * not us; and any subsequent ones cannot be because our + * caller must necessarily have obtained a snapshot later than + * the pg_upgrade itself. + */ + Assert(!HEAP_LOCKED_UPGRADED(mytup.t_data->t_infomask)); nmembers = GetMultiXactIdMembers(rawxmax, &members, false); for (i = 0; i < nmembers; i++) { @@ -5329,14 +5339,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, bool has_lockers; TransactionId update_xid; bool update_committed; - bool allow_old; *flags = 0; /* We should only be called in Multis */ Assert(t_infomask & HEAP_XMAX_IS_MULTI); - if (!MultiXactIdIsValid(multi)) + if (!MultiXactIdIsValid(multi) || + HEAP_LOCKED_UPGRADED(t_infomask)) { /* Ensure infomask bits are appropriately set/reset */ *flags |= FRM_INVALIDATE_XMAX; @@ -5349,14 +5359,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, * was a locker only, it can be removed without any further * consideration; but if it contained an update, we might need to * preserve it. - * - * Don't assert MultiXactIdIsRunning if the multi came from a - * pg_upgrade'd share-locked tuple, though, as doing that causes an - * error to be raised unnecessarily. */ - Assert((!(t_infomask & HEAP_LOCK_MASK) && - HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)) || - !MultiXactIdIsRunning(multi)); + Assert(!MultiXactIdIsRunning(multi)); if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)) { *flags |= FRM_INVALIDATE_XMAX; @@ -5397,9 +5401,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask, * anything. */ - allow_old = !(t_infomask & HEAP_LOCK_MASK) && - HEAP_XMAX_IS_LOCKED_ONLY(t_infomask); - nmembers = GetMultiXactIdMembers(multi, &members, allow_old); + nmembers = + GetMultiXactIdMembers(multi, &members, false); if (nmembers <= 0) { /* Nothing worth keeping */ @@ -5959,14 +5962,15 @@ static bool DoesMultiXactIdConflict(MultiXactId multi, uint16 infomask, LockTupleMode lockmode) { - bool allow_old; int nmembers; MultiXactMember *members; bool result = false; LOCKMODE wanted = tupleLockExtraInfo[lockmode].hwlock; - allow_old = !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(infomask); - nmembers = GetMultiXactIdMembers(multi, &members, allow_old); + if (HEAP_LOCKED_UPGRADED(infomask)) + return false; + + nmembers = GetMultiXactIdMembers(multi, &members, false); if (nmembers >= 0) { int i; @@ -6037,14 +6041,14 @@ static bool Do_MultiXactIdWait(MultiXactId multi, MultiXactStatus status, int *remaining, uint16 infomask, bool nowait) { - bool allow_old; bool result = true; MultiXactMember *members; int nmembers; int remain = 0; - allow_old = !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(infomask); - nmembers = GetMultiXactIdMembers(multi, &members, allow_old); + /* for pre-pg_upgrade tuples, no need to sleep at all */ + nmembers = HEAP_LOCKED_UPGRADED(infomask) ? -1 : + GetMultiXactIdMembers(multi, &members, false); if (nmembers >= 0) { @@ -6168,6 +6172,8 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid, /* no xmax set, ignore */ ; } + else if (HEAP_LOCKED_UPGRADED(tuple->t_infomask)) + return true; else if (MultiXactIdPrecedes(multi, cutoff_multi)) return true; else @@ -6175,13 +6181,9 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid, MultiXactMember *members; int nmembers; int i; - bool allow_old; /* need to check whether any member of the mxact is too old */ - - allow_old = !(tuple->t_infomask & HEAP_LOCK_MASK) && - HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask); - nmembers = GetMultiXactIdMembers(multi, &members, allow_old); + nmembers = GetMultiXactIdMembers(multi, &members, false); for (i = 0; i < nmembers; i++) { @@ -6858,7 +6860,8 @@ heap_xlog_freeze_tuple(HeapTupleHeader tuple, TransactionId cutoff_xid, xid = HeapTupleHeaderGetRawXmax(tuple); if ((tuple->t_infomask & HEAP_XMAX_IS_MULTI) ? (MultiXactIdIsValid(xid) && - MultiXactIdPrecedes(xid, cutoff_multi)) : + (HEAP_LOCKED_UPGRADED(tuple->t_infomask) || + MultiXactIdPrecedes(xid, cutoff_multi))) : (TransactionIdIsNormal(xid) && TransactionIdPrecedes(xid, cutoff_xid))) { diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 9b10496d9bd..cb7fab15627 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -1189,23 +1189,29 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset) /* * GetMultiXactIdMembers - * Returns the set of MultiXactMembers that make up a MultiXactId + * Return the set of MultiXactMembers that make up a MultiXactId * - * If the given MultiXactId is older than the value we know to be oldest, we - * return -1. The caller is expected to allow that only in permissible cases, - * i.e. when the infomask lets it presuppose that the tuple had been - * share-locked before a pg_upgrade; this means that the HEAP_XMAX_LOCK_ONLY - * needs to be set, but HEAP_XMAX_KEYSHR_LOCK and HEAP_XMAX_EXCL_LOCK are not - * set. + * Return value is the number of members found, or -1 if there are none, + * and *members is set to a newly palloc'ed array of members. It's the + * caller's responsibility to free it when done with it. * - * Other border conditions, such as trying to read a value that's larger than - * the value currently known as the next to assign, raise an error. Previously - * these also returned -1, but since this can lead to the wrong visibility - * results, it is dangerous to do that. + * from_pgupgrade must be passed as true if and only if only the multixact + * corresponds to a value from a tuple that was locked in a 9.2-or-older + * installation and later pg_upgrade'd (that is, the infomask is + * HEAP_LOCKED_UPGRADED). In this case, we know for certain that no members + * can still be running, so we return -1 just like for an empty multixact + * without any further checking. It would be wrong to try to resolve such a + * multixact: either the multixact is within the current valid multixact + * range, in which case the returned result would be bogus, or outside that + * range, in which case an error would be raised. + * + * In all other cases, the passed multixact must be within the known valid + * range, that is, greater to or equal than oldestMultiXactId, and less than + * nextMXact. Otherwise, an error is raised. */ int GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, - bool allow_old) + bool from_pgupgrade) { int pageno; int prev_pageno; @@ -1224,7 +1230,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, debug_elog3(DEBUG2, "GetMembers: asked for %u", multi); - if (!MultiXactIdIsValid(multi)) + if (!MultiXactIdIsValid(multi) || from_pgupgrade) return -1; /* See if the MultiXactId is in the local cache */ @@ -1244,18 +1250,11 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, * * An ID older than MultiXactState->oldestMultiXactId cannot possibly be * useful; it has already been removed, or will be removed shortly, by - * truncation. Returning the wrong values could lead to an incorrect - * visibility result. However, to support pg_upgrade we need to allow an - * empty set to be returned regardless, if the caller is willing to accept - * it; the caller is expected to check that it's an allowed condition - * (such as ensuring that the infomask bits set on the tuple are - * consistent with the pg_upgrade scenario). If the caller is expecting - * this to be called only on recently created multis, then we raise an - * error. + * truncation. If one is passed, an error is raised. * - * Conversely, an ID >= nextMXact shouldn't ever be seen here; if it is - * seen, it implies undetected ID wraparound has occurred. This raises a - * hard error. + * Also, an ID >= nextMXact shouldn't ever be seen here; if it is seen, it + * implies undetected ID wraparound has occurred. This raises a hard + * error. * * Shared lock is enough here since we aren't modifying any global state. * Acquire it just long enough to grab the current counter values. We may @@ -1271,7 +1270,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members, if (MultiXactIdPrecedes(multi, oldestMXact)) { - ereport(allow_old ? DEBUG1 : ERROR, + ereport(ERROR, (errcode(ERRCODE_INTERNAL_ERROR), errmsg("MultiXactId %u does no longer exist -- apparent wraparound", multi))); @@ -1443,7 +1442,7 @@ MultiXactHasRunningRemoteMembers(MultiXactId multi) int nmembers; int i; - nmembers = GetMultiXactIdMembers(multi, &members, true); + nmembers = GetMultiXactIdMembers(multi, &members, false); if (nmembers <= 0) return false; diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c index ccbbf6279a3..931e2fb51b7 100644 --- a/src/backend/utils/time/tqual.c +++ b/src/backend/utils/time/tqual.c @@ -701,7 +701,9 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, if (tuple->t_infomask & HEAP_XMAX_IS_MULTI) { - if (MultiXactHasRunningRemoteMembers(xmax)) + if (HEAP_LOCKED_UPGRADED(tuple->t_infomask)) + return HeapTupleMayBeUpdated; + else if (MultiXactHasRunningRemoteMembers(xmax)) return HeapTupleBeingUpdated; else return HeapTupleMayBeUpdated; @@ -725,6 +727,7 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, /* not LOCKED_ONLY, so it has to have an xmax */ Assert(TransactionIdIsValid(xmax)); + Assert(!HEAP_LOCKED_UPGRADED(tuple->t_infomask)); /* updating subtransaction must have aborted */ if (!TransactionIdIsCurrentTransactionId(xmax)) @@ -787,15 +790,12 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, CommandId curcid, { TransactionId xmax; + if (HEAP_LOCKED_UPGRADED(tuple->t_infomask)) + return HeapTupleMayBeUpdated; + if (HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask)) { - /* - * If it's only locked but neither EXCL_LOCK nor KEYSHR_LOCK is - * set, it cannot possibly be running. Otherwise need to check. - */ - if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK | - HEAP_XMAX_KEYSHR_LOCK)) && - MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))) + if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))) return HeapTupleBeingUpdated; SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); @@ -1401,25 +1401,20 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, TransactionId OldestXmin, * "Deleting" xact really only locked it, so the tuple is live in any * case. However, we should make sure that either XMAX_COMMITTED or * XMAX_INVALID gets set once the xact is gone, to reduce the costs of - * examining the tuple for future xacts. Also, marking dead - * MultiXacts as invalid here provides defense against MultiXactId - * wraparound (see also comments in heap_freeze_tuple()). + * examining the tuple for future xacts. */ if (!(tuple->t_infomask & HEAP_XMAX_COMMITTED)) { if (tuple->t_infomask & HEAP_XMAX_IS_MULTI) { /* - * If it's only locked but neither EXCL_LOCK nor KEYSHR_LOCK - * are set, it cannot possibly be running; otherwise have to - * check. + * If it's a pre-pg_upgrade tuple, the multixact cannot + * possibly be running; otherwise have to check. */ - if ((tuple->t_infomask & (HEAP_XMAX_EXCL_LOCK | - HEAP_XMAX_KEYSHR_LOCK)) && + if (!HEAP_LOCKED_UPGRADED(tuple->t_infomask) && MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple))) return HEAPTUPLE_LIVE; SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId); - } else { diff --git a/src/include/access/htup_details.h b/src/include/access/htup_details.h index f7af83c9b6d..e5d30981069 100644 --- a/src/include/access/htup_details.h +++ b/src/include/access/htup_details.h @@ -203,6 +203,31 @@ struct HeapTupleHeaderData (((infomask) & HEAP_XMAX_LOCK_ONLY) || \ (((infomask) & (HEAP_XMAX_IS_MULTI | HEAP_LOCK_MASK)) == HEAP_XMAX_EXCL_LOCK)) +/* + * A tuple that has HEAP_XMAX_IS_MULTI and HEAP_XMAX_LOCK_ONLY but neither of + * XMAX_EXCL_LOCK and XMAX_KEYSHR_LOCK must come from a tuple that was + * share-locked in 9.2 or earlier and then pg_upgrade'd. + * + * In 9.2 and prior, HEAP_XMAX_IS_MULTI was only set when there were multiple + * FOR SHARE lockers of that tuple. That set HEAP_XMAX_LOCK_ONLY (with a + * different name back then) but neither of HEAP_XMAX_EXCL_LOCK and + * HEAP_XMAX_KEYSHR_LOCK. That combination is no longer possible in 9.3 and + * up, so if we see that combination we know for certain that the tuple was + * locked in an earlier release; since all such lockers are gone (they cannot + * survive through pg_upgrade), such tuples can safely be considered not + * locked. + * + * We must not resolve such multixacts locally, because the result would be + * bogus, regardless of where they stand with respect to the current valid + * multixact range. + */ +#define HEAP_LOCKED_UPGRADED(infomask) \ +( \ + ((infomask) & HEAP_XMAX_IS_MULTI) && \ + ((infomask) & HEAP_XMAX_LOCK_ONLY) && \ + (((infomask) & (HEAP_XMAX_EXCL_LOCK | HEAP_XMAX_KEYSHR_LOCK)) == 0) \ +) + /* * Use these to test whether a particular lock is applied to a tuple */