From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Andrew Gierth <andrew@tao11.riddles.org.uk>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: pgsql-hackers@postgresql.org
Subject: Re: MultiXactId error after upgrade to 9.3.4
Date: Thu, 16 Jun 2016 23:31:22 -0400
Message-ID: <20160617033122.GA106332@alvherre.pgsql> (raw)
In-Reply-To: <878ty5seyr.fsf@news-spur.riddles.org.uk>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Andrew Gierth wrote:
> But that leaves an obvious third issue: it's all very well to ignore the
> pre-upgrade (pre-9.3) mxid if it's older than the cutoff or it's in the
> future, but what if it's actually inside the currently valid range?
> Looking it up as though it were a valid mxid in that case seems to be
> completely wrong and could introduce more subtle errors.
You're right, we should not try to resolve a multixact coming from the
old install in any case.
> (It can, AFAICT, be inside the currently valid range due to wraparound,
> i.e. without there being a valid pg_multixact entry for it, because
> AFAICT in 9.2, once the mxid is hinted dead it is never again either
> looked up or cleared, so it can sit in the tuple xmax forever, even
> through multiple wraparounds.)
HeapTupleSatisfiesVacuum removes very old multixacts; see the
HEAP_IS_LOCKED block starting in line 1162 where we set
HEAP_XMAX_INVALID for multixacts that are not running by falling
through. It's not a strict guarantee but this probably explains why
this problem is not more common.
> 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.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackersAttachments:
[text/x-diff] upgraded-locks.patch (10.9K, ../20160617033122.GA106332@alvherre.pgsql/2-upgraded-locks.patch)
download | inline diff:diff --git a/contrib/pgrowlocks/pgrowlocks.c b/contrib/pgrowlocks/pgrowlocks.cindex 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.cindex 22b3f5f..9d2de7f 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,7 @@ l4:
int i;
MultiXactMember *members;
+ /* XXX do we need a HEAP_LOCKED_UPGRADED test? */
nmembers = GetMultiXactIdMembers(rawxmax, &members, false,
HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
for (i = 0; i < nmembers; i++)
@@ -6144,14 +6144,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 +6164,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 +6207,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 +6769,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 +6860,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 +7046,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 +7058,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.cindex 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.cindex 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.hindex 9f9cf2a..74da10c 100644--- a/src/include/access/htup_details.h+++ b/src/include/access/htup_details.h@@ -218,6 +218,20 @@ 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.+ *+ * 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.+ */+#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) \
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: alvherre@2ndquadrant.com, andrew@tao11.riddles.org.uk, sfrost@snowman.net
Subject: Re: MultiXactId error after upgrade to 9.3.4
In-Reply-To: <20160617033122.GA106332@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox