Robert Haas wrote:
> I see the patch, but I don't see much explanation of why the patch is
> correct, which I think is pretty scary in view of the number of
> mistakes we've already made in this area. The comments just say:
>
> + * 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.
>
> Why must that be true?
>
> + * We must not try to resolve such multixacts locally, because the result
> would
> + * be bogus, regardless of where they stand with respect to the current valid
> + * range.
>
> What about other pre-upgrade mxacts that don't have this exact bit
> pattern? Why can't we try to resolve them and end up in trouble just
> as easily?
Attached are two patches, one for 9.3 and 9.4 and the other for 9.5 and
master. Pretty much the same as before, but with answers to the above
concerns. In particular,
/*
+ * 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) \
+)
One place that had an XXX comment in the previous patch
(heap_lock_updated_tuple_rec) now contains this:
+ /*
+ * 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));
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
commit 7235c0b120208d73d53d3929fe8243d5e487dca8[m
Author: Alvaro Herrera <alvhe...@alvh.no-ip.org>
AuthorDate: Thu Jun 16 23:33:20 2016 -0400
CommitDate: Wed Jun 22 17:17:15 2016 -0400
Fix handling of multixacts predating pg_upgrade
After pg_upgrade, it is possible that some tuples' Xmax have multixacts
corresponding to the old installation; such multixacts cannot have
running members anymore. In many code sites we already know not to read
them and clobber them silently, but at least when VACUUM tries to freeze
a multixact or determine if one needs freezing, there's an attempt to
resolve it to its member transactions by calling GetMultiXactIdMembers,
and if the multixact value is "in the future" with regards to the
current valid multixact range, an error like this is raised:
ERROR: MultiXactId 123 has not been created yet -- apparent wraparound
and vacuuming fails. Per discussion with Andrew Gierth, it is completely
bogus to try to resolve multixacts coming from before a pg_upgrade,
regardless of where they stand with regards to the current valid
multixact range.
It's possible to get from under this problem by doing SELECT FOR UPDATE
of the problem tuples, but if tables are large, this is slow and
tedious, so a more thorough solution is desirable.
To fix, we realize that multixacts in xmax created in 9.2 and previous
have a specific bit pattern that is never used in 9.3 and later.
Whenever the infomask of the tuple matches that bit pattern, we just
ignore the multixact completely as if Xmax wasn't set; or, in the case
of tuple freezing, we act as if an unwanted value is set and clobber it
without decoding. This guarantees that no errors will be raised, and
that the values will be progressively removed until all tables are
clean. Most callers of GetMultiXactIdMembers are patched to recognize
directly that the value is a removable "empty" multixact and avoid
calling GetMultiXactIdMembers altogether.
To avoid changing the signature of GetMultiXactIdMembers() in back
branches, we keep the "allow_old" boolean flag but rename it to
"from_pgupgrade"; if the flag is true, we always return an empty set
instead of looking up the multixact. (We could remove the argument in
the master branch, but choose not to do so in this commit).
Bug analysis by Andrew Gierth and Álvaro Herrera.
A number of public reports match this bug:
https://www.postgresql.org/message-id/20140330040029.gy4...@tamriel.snowman.net
https://www.postgresql.org/message-id/538f3d70.6080...@publicrelay.com
https://www.postgresql.org/message-id/556439cf.7070...@pscs.co.uk
https://www.postgresql.org/message-id/sg2pr06mb0760098a111c88e31bd4d96fb3...@sg2pr06mb0760.apcprd06.prod.outlook.com
https://www.postgresql.org/message-id/20160615203829.5798.4...@wrigleys.postgresql.org
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 075d781..ee5b241 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 1f1bac5..89a9335 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4641,8 +4641,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 +5000,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 +5338,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 +5358,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 +5400,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 +5961,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 +6040,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)
{
@@ -6165,9 +6168,11 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid,
multi = HeapTupleHeaderGetRawXmax(tuple);
if (!MultiXactIdIsValid(multi))
{
- /* no xmax set, ignore */
+ /* no valid xmax set, ignore */
;
}
+ else if (HEAP_LOCKED_UPGRADED(tuple->t_infomask))
+ return true;
else if (MultiXactIdPrecedes(multi, cutoff_multi))
return true;
else
@@ -6175,13 +6180,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++)
{
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 9b10496..efbca6f 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1191,12 +1191,17 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
* GetMultiXactIdMembers
* Returns 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.
+ * from_pgupgrade should be set to true when a multixact corresponds to a
+ * value from a tuple that was locked in a 9.2-or-older install and later
+ * pg_upgrade'd. In this case, we now for certain that no members can still
+ * be running, so we return -1 just like for an empty multixact without
+ * further any further checking. It would be wrong to try to resolve such
+ * multixacts, because they may be pointing to any part of the multixact
+ * space, both within the current valid range in which case we could return
+ * bogus results, or outside it, which would raise errors. This flag should
+ * only be passed true when the multixact is attached to a tuple with
+ * HEAP_XMAX_LOCK_ONLY set, but neither of HEAP_XMAX_KEYSHR_LOCK and
+ * HEAP_XMAX_EXCL_LOCK.
*
* 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
@@ -1205,7 +1210,7 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
*/
int
GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
- bool allow_old)
+ bool from_pgupgrade)
{
int pageno;
int prev_pageno;
@@ -1224,7 +1229,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 */
@@ -1271,7 +1276,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 +1448,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 ccbbf62..931e2fb 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 f7af83c..e5d3098 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -204,6 +204,31 @@ struct HeapTupleHeaderData
(((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
*/
#define HEAP_XMAX_IS_SHR_LOCKED(infomask) \
commit a97b3b0300442eb0125f0cb3dd179b420b411b92[m
Author: Alvaro Herrera <alvhe...@alvh.no-ip.org>
AuthorDate: Thu Jun 16 23:33:20 2016 -0400
CommitDate: Wed Jun 22 15:25:07 2016 -0400
Fix handling of multixacts predating pg_upgrade
After pg_upgrade, it is possible that some tuples' Xmax have multixacts
corresponding to the old installation; such multixacts cannot have
running members anymore. In many code sites we already know not to read
them and clobber them silently, but at least when VACUUM tries to freeze
a multixact or determine if one needs freezing, there's an attempt to
resolve it to its member transactions by calling GetMultiXactIdMembers,
and if the multixact value is "in the future" with regards to the
current valid multixact range, an error like this is raised:
ERROR: MultiXactId 123 has not been created yet -- apparent wraparound
and vacuuming fails. Per discussion with Andrew Gierth, it is completely
bogus to try to resolve multixacts coming from before a pg_upgrade,
regardless of where they stand with regards to the current valid
multixact range.
It's possible to get from under this problem by doing SELECT FOR UPDATE
of the problem tuples, but if tables are large, this is slow and
tedious, so a more thorough solution is desirable.
To fix, we realize that multixacts in xmax created in 9.2 and previous
have a specific bit pattern that is never used in 9.3 and later.
Whenever the infomask of the tuple matches that bit pattern, we just
ignore the multixact completely as if Xmax wasn't set; or, in the case
of tuple freezing, we act as if an unwanted value is set and clobber it
without decoding. This guarantees that no errors will be raised, and
that the values will be progressively removed until all tables are
clean. Most callers of GetMultiXactIdMembers are patched to recognize
directly that the value is a removable "empty" multixact and avoid
calling GetMultiXactIdMembers altogether.
To avoid changing the signature of GetMultiXactIdMembers() in back
branches, we keep the "allow_old" boolean flag but rename it to
"from_pgupgrade"; if the flag is true, we always return an empty set
instead of looking up the multixact. (We could remove the argument in
the master branch, but choose not to do so in this commit).
Bug analysis by Andrew Gierth and Álvaro Herrera.
A number of public reports match this bug:
https://www.postgresql.org/message-id/20140330040029.gy4...@tamriel.snowman.net
https://www.postgresql.org/message-id/538f3d70.6080...@publicrelay.com
https://www.postgresql.org/message-id/556439cf.7070...@pscs.co.uk
https://www.postgresql.org/message-id/sg2pr06mb0760098a111c88e31bd4d96fb3...@sg2pr06mb0760.apcprd06.prod.outlook.com
https://www.postgresql.org/message-id/20160615203829.5798.4...@wrigleys.postgresql.org
diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.c
index 88f7137..e20e7f8 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -158,8 +158,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,
false);
if (nmembers == -1)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 80e9594..24416c6 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5225,8 +5225,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;
@@ -5586,6 +5585,17 @@ 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,
HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
for (i = 0; i < nmembers; i++)
@@ -6144,14 +6154,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;
@@ -6164,14 +6174,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,
HEAP_XMAX_IS_LOCKED_ONLY(t_infomask)));
if (HEAP_XMAX_IS_LOCKED_ONLY(t_infomask))
{
@@ -6213,10 +6217,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,
+ GetMultiXactIdMembers(multi, &members, false,
HEAP_XMAX_IS_LOCKED_ONLY(t_infomask));
if (nmembers <= 0)
{
@@ -6777,14 +6779,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,
HEAP_XMAX_IS_LOCKED_ONLY(infomask));
if (nmembers >= 0)
{
@@ -6867,15 +6870,15 @@ Do_MultiXactIdWait(MultiXactId multi, MultiXactStatus status,
Relation rel, ItemPointer ctid, XLTW_Oper oper,
int *remaining)
{
- 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,
- HEAP_XMAX_IS_LOCKED_ONLY(infomask));
+ /* for pre-pg_upgrade tuples, no need to sleep at all */
+ nmembers = HEAP_LOCKED_UPGRADED(infomask) ? -1 :
+ GetMultiXactIdMembers(multi, &members, false,
+ HEAP_XMAX_IS_LOCKED_ONLY(infomask));
if (nmembers >= 0)
{
@@ -7053,9 +7056,11 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid,
multi = HeapTupleHeaderGetRawXmax(tuple);
if (!MultiXactIdIsValid(multi))
{
- /* no xmax set, ignore */
+ /* no valid xmax set, ignore */
;
}
+ else if (HEAP_LOCKED_UPGRADED(tuple->t_infomask))
+ return true;
else if (MultiXactIdPrecedes(multi, cutoff_multi))
return true;
else
@@ -7063,13 +7068,10 @@ 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,
HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask));
for (i = 0; i < nmembers; i++)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 7bccca8..ee2b7ab 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1175,12 +1175,17 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
* GetMultiXactIdMembers
* Returns 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.
+ * from_pgupgrade should be set to true when a multixact corresponds to a
+ * value from a tuple that was locked in a 9.2-or-older install and later
+ * pg_upgrade'd. In this case, we now for certain that no members can still
+ * be running, so we return -1 just like for an empty multixact without
+ * further any further checking. It would be wrong to try to resolve such
+ * multixacts, because they may be pointing to any part of the multixact
+ * space, both within the current valid range in which case we could return
+ * bogus results, or outside it, which would raise errors. This flag should
+ * only be passed true when the multixact is attached to a tuple with
+ * HEAP_XMAX_LOCK_ONLY set, but neither of HEAP_XMAX_KEYSHR_LOCK and
+ * HEAP_XMAX_EXCL_LOCK.
*
* 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
@@ -1194,7 +1199,7 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offset)
*/
int
GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
- bool allow_old, bool onlyLock)
+ bool from_pgupgrade, bool onlyLock)
{
int pageno;
int prev_pageno;
@@ -1213,7 +1218,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 */
@@ -1273,7 +1278,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)));
diff --git a/src/backend/utils/time/tqual.c b/src/backend/utils/time/tqual.c
index 0e815a9..a08dae1 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -620,15 +620,12 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, 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), true))
+ if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple), true))
return HeapTupleBeingUpdated;
SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
@@ -1279,26 +1276,21 @@ HeapTupleSatisfiesVacuum(HeapTuple htup, 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),
true))
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 9f9cf2a..d7e5fad 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -218,6 +218,31 @@ struct HeapTupleHeaderData
(((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
*/
#define HEAP_XMAX_IS_SHR_LOCKED(infomask) \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
