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
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 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.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..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) \
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
