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
commit 5a57daa6ef9784c42f3cb2ec6e3a58d1e4004593[m
Author: Alvaro Herrera <alvhe...@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/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..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 &= ~HEAP_XMAX_IS_MULTI;
old_infomask |= HEAP_XMAX_INVALID;
@@ -5001,6 +5000,7 @@ l4:
int i;
MultiXactMember *members;
+ /* XXX do we need a HEAP_LOCKED_UPGRADED test? */
nmembers = GetMultiXactIdMembers(rawxmax, &members, false);
for (i = 0; i < nmembers; i++)
{
@@ -5329,14 +5329,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 +5349,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 +5391,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 +5952,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 +6031,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 +6159,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 +6171,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..15de62d 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)));
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, 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 +1398,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..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)) == 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) \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
