Date: Fri, 17 Jun 2016 18:55:17 -0400
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andrew Gierth <andrew@tao11.riddles.org.uk>
Cc: Stephen Frost <sfrost@snowman.net>, pgsql-hackers@postgresql.org
Subject: Re: MultiXactId error after upgrade to 9.3.4
Message-ID: <20160617225517.GA157662@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="5vNYLRcllDrimb99"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20160617033122.GA106332@alvherre.pgsql>
User-Agent: Mutt/1.5.23 (2014-03-12)
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org


--5vNYLRcllDrimb99
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Alvaro Herrera wrote:
> Andrew Gierth wrote:

> > Why is the correct rule not "check for and ignore pre-upgrade mxids
> > before even trying to fetch members"?
> 
> I propose something like the attached patch, which implements that idea.

Here's a backpatch of that to 9.3 and 9.4.

I tested this by creating a 9.2 installation with an out-of-range
multixact, and verified that after upgrading this to 9.3 it fails with
the "apparent wraparound" message in VACUUM.  With this patch applied,
it silently removes the multixact.

I will clean this up and push to all branches after the tagging of
9.6beta2 on Monday.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--5vNYLRcllDrimb99
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="upgraded-locks-93.patch"
Content-Transfer-Encoding: quoted-printable

commit 5a57daa6ef9784c42f3cb2ec6e3a58d1e4004593=1B[m
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org>
AuthorDate: Thu Jun 16 23:33:20 2016 -0400
CommitDate: Fri Jun 17 18:46:04 2016 -0400

    Fix upgraded multixact problem

diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlock=
s.c
index 075d781..ee5b241 100644
--- a/contrib/pgrowlocks/pgrowlocks.c
+++ b/contrib/pgrowlocks/pgrowlocks.c
@@ -165,8 +165,7 @@ pgrowlocks(PG_FUNCTION_ARGS)
=20
 				values[Atnum_ismulti] =3D pstrdup("true");
=20
-				allow_old =3D !(infomask & HEAP_LOCK_MASK) &&
-					(infomask & HEAP_XMAX_LOCK_ONLY);
+				allow_old =3D HEAP_LOCKED_UPGRADED(infomask);
 				nmembers =3D GetMultiXactIdMembers(xmax, &members, allow_old);
 				if (nmembers =3D=3D -1)
 				{
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/hea=
pam.c
index 1f1bac5..44aa495 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 &=3D ~HEAP_XMAX_IS_MULTI;
 			old_infomask |=3D HEAP_XMAX_INVALID;
@@ -5001,6 +5000,7 @@ l4:
 				int		i;
 				MultiXactMember *members;
=20
+				/* XXX do we need a HEAP_LOCKED_UPGRADED test? */
 				nmembers =3D GetMultiXactIdMembers(rawxmax, &members, false);
 				for (i =3D 0; i < nmembers; i++)
 				{
@@ -5329,14 +5329,14 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infom=
ask,
 	bool		has_lockers;
 	TransactionId update_xid;
 	bool		update_committed;
-	bool		allow_old;
=20
 	*flags =3D 0;
=20
 	/* We should only be called in Multis */
 	Assert(t_infomask & HEAP_XMAX_IS_MULTI);
=20
-	if (!MultiXactIdIsValid(multi))
+	if (!MultiXactIdIsValid(multi) ||
+		HEAP_LOCKED_UPGRADED(t_infomask))
 	{
 		/* Ensure infomask bits are appropriately set/reset */
 		*flags |=3D FRM_INVALIDATE_XMAX;
@@ -5349,14 +5349,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infoma=
sk,
 		 * 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 |=3D FRM_INVALIDATE_XMAX;
@@ -5397,9 +5391,8 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomas=
k,
 	 * anything.
 	 */
=20
-	allow_old =3D !(t_infomask & HEAP_LOCK_MASK) &&
-		HEAP_XMAX_IS_LOCKED_ONLY(t_infomask);
-	nmembers =3D GetMultiXactIdMembers(multi, &members, allow_old);
+	nmembers =3D
+		GetMultiXactIdMembers(multi, &members, false);
 	if (nmembers <=3D 0)
 	{
 		/* Nothing worth keeping */
@@ -5959,14 +5952,15 @@ static bool
 DoesMultiXactIdConflict(MultiXactId multi, uint16 infomask,
 						LockTupleMode lockmode)
 {
-	bool	allow_old;
 	int		nmembers;
 	MultiXactMember *members;
 	bool	result =3D false;
 	LOCKMODE wanted =3D tupleLockExtraInfo[lockmode].hwlock;
=20
-	allow_old =3D !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(in=
fomask);
-	nmembers =3D GetMultiXactIdMembers(multi, &members, allow_old);
+	if (HEAP_LOCKED_UPGRADED(infomask))
+		return false;
+
+	nmembers =3D GetMultiXactIdMembers(multi, &members, false);
 	if (nmembers >=3D 0)
 	{
 		int		i;
@@ -6037,14 +6031,14 @@ static bool
 Do_MultiXactIdWait(MultiXactId multi, MultiXactStatus status,
 				   int *remaining, uint16 infomask, bool nowait)
 {
-	bool		allow_old;
 	bool		result =3D true;
 	MultiXactMember *members;
 	int			nmembers;
 	int			remain =3D 0;
=20
-	allow_old =3D !(infomask & HEAP_LOCK_MASK) && HEAP_XMAX_IS_LOCKED_ONLY(in=
fomask);
-	nmembers =3D GetMultiXactIdMembers(multi, &members, allow_old);
+	/* for pre-pg_upgrade tuples, no need to sleep at all */
+	nmembers =3D HEAP_LOCKED_UPGRADED(infomask) ? -1 :
+		GetMultiXactIdMembers(multi, &members, false);
=20
 	if (nmembers >=3D 0)
 	{
@@ -6165,9 +6159,11 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, Trans=
actionId cutoff_xid,
 		multi =3D 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 +6171,9 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, Trans=
actionId cutoff_xid,
 			MultiXactMember *members;
 			int			nmembers;
 			int			i;
-			bool		allow_old;
=20
 			/* need to check whether any member of the mxact is too old */
-
-			allow_old =3D !(tuple->t_infomask & HEAP_LOCK_MASK) &&
-				HEAP_XMAX_IS_LOCKED_ONLY(tuple->t_infomask);
-			nmembers =3D GetMultiXactIdMembers(multi, &members, allow_old);
+			nmembers =3D GetMultiXactIdMembers(multi, &members, false);
=20
 			for (i =3D 0; i < nmembers; i++)
 			{
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/tr=
ansam/multixact.c
index 9b10496..15de62d 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1191,12 +1191,17 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *of=
fset)
  * 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 ca=
ses,
- * 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_ON=
LY
- * 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 sti=
ll
+ * 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 shou=
ld
+ * 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 t=
han
  * the value currently known as the next to assign, raise an error.  Previ=
ously
@@ -1205,7 +1210,7 @@ GetNewMultiXactId(int nmembers, MultiXactOffset *offs=
et)
  */
 int
 GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
-					  bool allow_old)
+					  bool from_pgupgrade)
 {
 	int			pageno;
 	int			prev_pageno;
@@ -1224,7 +1229,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMem=
ber **members,
=20
 	debug_elog3(DEBUG2, "GetMembers: asked for %u", multi);
=20
-	if (!MultiXactIdIsValid(multi))
+	if (!MultiXactIdIsValid(multi) || from_pgupgrade)
 		return -1;
=20
 	/* See if the MultiXactId is in the local cache */
@@ -1271,7 +1276,7 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMem=
ber **members,
=20
 	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 ccbbf62..9d7050a 100644
--- a/src/backend/utils/time/tqual.c
+++ b/src/backend/utils/time/tqual.c
@@ -787,15 +787,12 @@ HeapTupleSatisfiesUpdate(HeapTupleHeader tuple, Comma=
ndId curcid,
 	{
 		TransactionId xmax;
=20
+		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;
=20
 			SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
@@ -1401,25 +1398,20 @@ HeapTupleSatisfiesVacuum(HeapTupleHeader tuple, Tra=
nsactionId 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_de=
tails.h
index f7af83c..1cdbc64 100644
--- a/src/include/access/htup_details.h
+++ b/src/include/access/htup_details.h
@@ -204,6 +204,20 @@ struct HeapTupleHeaderData
 	 (((infomask) & (HEAP_XMAX_IS_MULTI | HEAP_LOCK_MASK)) =3D=3D HEAP_XMAX_E=
XCL_LOCK))
=20
 /*
+ * 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.
+ *
+ * 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 va=
lid
+ * 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)) =3D=3D 0))
+
+/*
  * Use these to test whether a particular lock is applied to a tuple
  */
 #define HEAP_XMAX_IS_SHR_LOCKED(infomask) \

--5vNYLRcllDrimb99
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--5vNYLRcllDrimb99--