Fix handling of multixacts predating pg_upgrade
authorAlvaro Herrera
Fri, 24 Jun 2016 22:29:28 +0000 (18:29 -0400)
committerAlvaro Herrera
Fri, 24 Jun 2016 22:29:28 +0000 (18:29 -0400)
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 whether 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 (we
already knew this, per comments and infomask tests sprinkled in various
places, but we weren't leveraging this knowledge appropriately).
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.  (I suppose we could remove the
argument in the master branch, but I chose not to do so in this commit).

This was broken all along, but the error-facing message appeared first
because of commit 8e9a16ab8f7f and was partially fixed in a25c2b7c4db3.
This fix, backpatched all the way back to 9.3, goes approximately in the
same direction as a25c2b7c4db3 but should cover all cases.

Bug analysis by Andrew Gierth and Álvaro Herrera.

A number of public reports match this bug:
  https://www.postgresql.org/message-id/20140330040029[email protected]
  https://www.postgresql.org/message-id/538F3D70.6080902@publicrelay.com
  https://www.postgresql.org/message-id/556439CF.7070109@pscs.co.uk
  https://www.postgresql.org/message-id/SG2PR06MB0760098A111C88E31BD4D96FB3540@SG2PR06MB0760.apcprd06.prod.outlook.com
  https://www.postgresql.org/message-id/20160615203829[email protected]

contrib/pgrowlocks/pgrowlocks.c
src/backend/access/heap/heapam.c
src/backend/access/transam/multixact.c
src/backend/utils/time/tqual.c
src/include/access/htup_details.h

index 15d97047525c88b7198cdd8912715b8377afb9a4..286b9a2e92ec9fb20e9799e7f6f15c0be70efdae 100644 (file)
@@ -164,8 +164,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)
                {
index de51cf7eec0eb46d86018682a8014ab8ce5d0cb2..eea507bc90e6ed7f45483c79ad032435d115c20c 100644 (file)
@@ -3509,6 +3509,7 @@ l2:
     * HEAP_XMAX_INVALID bit set; that's fine.)
     */
    if ((oldtup.t_data->t_infomask & HEAP_XMAX_INVALID) ||
+       HEAP_LOCKED_UPGRADED(oldtup.t_data->t_infomask) ||
        (checked_lockers && !locker_remains))
        xmax_new_tuple = InvalidTransactionId;
    else
@@ -4833,8 +4834,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;
@@ -5194,6 +5194,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++)
                {
@@ -5525,14 +5535,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;
@@ -5545,14 +5555,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;
@@ -5593,9 +5597,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 */
@@ -6146,14 +6149,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;
@@ -6235,14 +6239,14 @@ 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);
+   /* for pre-pg_upgrade tuples, no need to sleep at all */
+   nmembers = HEAP_LOCKED_UPGRADED(infomask) ? -1 :
+       GetMultiXactIdMembers(multi, &members, false);
 
    if (nmembers >= 0)
    {
@@ -6374,6 +6378,8 @@ heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_xid,
            /* no xmax set, ignore */
            ;
        }
+       else if (HEAP_LOCKED_UPGRADED(tuple->t_infomask))
+           return true;
        else if (MultiXactIdPrecedes(multi, cutoff_multi))
            return true;
        else
@@ -6381,13 +6387,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++)
            {
index fd7b34fe4b590653963adabfe8d275b6526966f0..b2ea28c67fd6f15a1a78e6f35de5c3aef4fc46e7 100644 (file)
@@ -1189,23 +1189,29 @@ 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.
- *
- * 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
- * these also returned -1, but since this can lead to the wrong visibility
- * results, it is dangerous to do that.
+ *     Return the set of MultiXactMembers that make up a MultiXactId
+ *
+ * Return value is the number of members found, or -1 if there are none,
+ * and *members is set to a newly palloc'ed array of members.  It's the
+ * caller's responsibility to free it when done with it.
+ *
+ * from_pgupgrade must be passed as true if and only if only the multixact
+ * corresponds to a value from a tuple that was locked in a 9.2-or-older
+ * installation and later pg_upgrade'd (that is, the infomask is
+ * HEAP_LOCKED_UPGRADED).  In this case, we know for certain that no members
+ * can still be running, so we return -1 just like for an empty multixact
+ * without any further checking.  It would be wrong to try to resolve such a
+ * multixact: either the multixact is within the current valid multixact
+ * range, in which case the returned result would be bogus, or outside that
+ * range, in which case an error would be raised.
+ *
+ * In all other cases, the passed multixact must be within the known valid
+ * range, that is, greater to or equal than oldestMultiXactId, and less than
+ * nextMXact.  Otherwise, an error is raised.
  */
 int
 GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
-                     bool allow_old)
+                     bool from_pgupgrade)
 {
    int         pageno;
    int         prev_pageno;
@@ -1224,7 +1230,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 */
@@ -1244,18 +1250,11 @@ GetMultiXactIdMembers(MultiXactId multi, MultiXactMember **members,
     *
     * An ID older than MultiXactState->oldestMultiXactId cannot possibly be
     * useful; it has already been removed, or will be removed shortly, by
-    * truncation.  Returning the wrong values could lead to an incorrect
-    * visibility result.  However, to support pg_upgrade we need to allow an
-    * empty set to be returned regardless, if the caller is willing to accept
-    * it; the caller is expected to check that it's an allowed condition
-    * (such as ensuring that the infomask bits set on the tuple are
-    * consistent with the pg_upgrade scenario).  If the caller is expecting
-    * this to be called only on recently created multis, then we raise an
-    * error.
+    * truncation.  If one is passed, an error is raised.
     *
-    * Conversely, an ID >= nextMXact shouldn't ever be seen here; if it is
-    * seen, it implies undetected ID wraparound has occurred.  This raises a
-    * hard error.
+    * Also, an ID >= nextMXact shouldn't ever be seen here; if it is seen, it
+    * implies undetected ID wraparound has occurred.  This raises a hard
+    * error.
     *
     * Shared lock is enough here since we aren't modifying any global state.
     * Acquire it just long enough to grab the current counter values.  We may
@@ -1271,7 +1270,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 +1442,7 @@ MultiXactHasRunningRemoteMembers(MultiXactId multi)
    int         nmembers;
    int         i;
 
-   nmembers = GetMultiXactIdMembers(multi, &members, true);
+   nmembers = GetMultiXactIdMembers(multi, &members, false);
    if (nmembers <= 0)
        return false;
 
index 96874ab80a7882f5869ba514530494ed252af832..773255c29b603549a9ee82609827a0fec084c5dd 100644 (file)
@@ -513,7 +513,9 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, 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;
@@ -537,6 +539,7 @@ HeapTupleSatisfiesUpdate(HeapTuple htup, 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))
@@ -599,15 +602,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)))
+           if (MultiXactIdIsRunning(HeapTupleHeaderGetRawXmax(tuple)))
                return HeapTupleBeingUpdated;
 
            SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
@@ -1225,25 +1225,20 @@ 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)))
                    return HEAPTUPLE_LIVE;
                SetHintBits(tuple, buffer, HEAP_XMAX_INVALID, InvalidTransactionId);
-
            }
            else
            {
index 294d21bd18080333d3676eea4c08f53307e56a7c..a4efcf4ec42a4a4af710f0d41e3b35af8a9f5e80 100644 (file)
@@ -205,6 +205,31 @@ struct HeapTupleHeaderData
    (((infomask) & HEAP_XMAX_LOCK_ONLY) || \
     (((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
  */