Optimize updating a row that's locked by same xid

Updating or locking a row that was already locked by the same
transaction under the same Xid caused a MultiXact to be created; but
this is unnecessary, because there's no usefulness in being able to
differentiate two locks by the same transaction.  In particular, if a
transaction executed SELECT FOR UPDATE followed by an UPDATE that didn't
modify columns of the key, we would dutifully represent the resulting
combination as a multixact -- even though a single key-update is
sufficient.

Optimize the case so that only the strongest of both locks/updates is
represented in Xmax.  This can save some Xmax's from becoming
MultiXacts, which can be a significant optimization.

This missed optimization opportunity was spotted by Andres Freund while
investigating a bug reported by Oliver Seemann in message
CANCipfpfzoYnOz5jj=UZ70_R=CwDHv36dqWSpwsi27vpm1z5sA@mail.gmail.com
and also directly as a performance regression reported by Dong Ye in
message
d54b8387.000012d8.00000010@YED-DEVD1.vmware.com
Reportedly, this patch fixes the performance regression.

Since the missing optimization was reported as a significant performance
regression from 9.2, backpatch to 9.3.

Andres Freund, tweaked by Álvaro Herrera

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index db683b121796309155ed99f0517a1a91572bdb9b..4dfc8b1de50fa938586e6003948d2cb9ae7403f3 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4705,6 +4705,8 @@ compute_new_xmax_infomask(TransactionId xmax, uint16 old_infomask,
    uint16      new_infomask,
                new_infomask2;
 
+   Assert(TransactionIdIsCurrentTransactionId(add_to_xmax));
+
 l5:
    new_infomask = 0;
    new_infomask2 = 0;
@@ -4712,6 +4714,11 @@ l5:
    {
        /*
         * No previous locker; we just insert our own TransactionId.
+        *
+        * Note that it's critical that this case be the first one checked,
+        * because there are several blocks below that come back to this one
+        * to implement certain optimizations; old_infomask might contain
+        * other dirty bits in those cases, but we don't really care.
         */
        if (is_update)
        {
@@ -4837,21 +4844,22 @@ l5:
         * create a new MultiXactId that includes both the old locker or
         * updater and our own TransactionId.
         */
-       MultiXactStatus status;
        MultiXactStatus new_status;
+       MultiXactStatus old_status;
+       LockTupleMode   old_mode;
 
        if (HEAP_XMAX_IS_LOCKED_ONLY(old_infomask))
        {
            if (HEAP_XMAX_IS_KEYSHR_LOCKED(old_infomask))
-               status = MultiXactStatusForKeyShare;
+               old_status = MultiXactStatusForKeyShare;
            else if (HEAP_XMAX_IS_SHR_LOCKED(old_infomask))
-               status = MultiXactStatusForShare;
+               old_status = MultiXactStatusForShare;
            else if (HEAP_XMAX_IS_EXCL_LOCKED(old_infomask))
            {
                if (old_infomask2 & HEAP_KEYS_UPDATED)
-                   status = MultiXactStatusForUpdate;
+                   old_status = MultiXactStatusForUpdate;
                else
-                   status = MultiXactStatusForNoKeyUpdate;
+                   old_status = MultiXactStatusForNoKeyUpdate;
            }
            else
            {
@@ -4871,43 +4879,43 @@ l5:
        {
            /* it's an update, but which kind? */
            if (old_infomask2 & HEAP_KEYS_UPDATED)
-               status = MultiXactStatusUpdate;
+               old_status = MultiXactStatusUpdate;
            else
-               status = MultiXactStatusNoKeyUpdate;
+               old_status = MultiXactStatusNoKeyUpdate;
        }
 
-       new_status = get_mxact_status_for_lock(mode, is_update);
+       old_mode = TUPLOCK_from_mxstatus(old_status);
 
        /*
-        * If the existing lock mode is identical to or weaker than the new
-        * one, we can act as though there is no existing lock, so set
-        * XMAX_INVALID and restart.
+        * If the lock to be acquired is for the same TransactionId as the
+        * existing lock, there's an optimization possible: consider only the
+        * strongest of both locks as the only one present, and restart.
         */
        if (xmax == add_to_xmax)
        {
-           LockTupleMode old_mode = TUPLOCK_from_mxstatus(status);
-           bool        old_isupd = ISUPDATE_from_mxstatus(status);
-
            /*
-            * We can do this if the new LockTupleMode is higher or equal than
-            * the old one; and if there was previously an update, we need an
-            * update, but if there wasn't, then we can accept there not being
-            * one.
+            * Note that it's not possible for the original tuple to be updated:
+            * we wouldn't be here because the tuple would have been invisible and
+            * we wouldn't try to update it.  As a subtlety, this code can also
+            * run when traversing an update chain to lock future versions of a
+            * tuple.  But we wouldn't be here either, because the add_to_xmax
+            * would be different from the original updater.
             */
-           if ((mode >= old_mode) && (is_update || !old_isupd))
-           {
-               /*
-                * Note that the infomask might contain some other dirty bits.
-                * However, since the new infomask is reset to zero, we only
-                * set what's minimally necessary, and that the case that
-                * checks HEAP_XMAX_INVALID is the very first above, there is
-                * no need for extra cleanup of the infomask here.
-                */
-               old_infomask |= HEAP_XMAX_INVALID;
-               goto l5;
-           }
+           Assert(HEAP_XMAX_IS_LOCKED_ONLY(old_infomask));
+
+           /* acquire the strongest of both */
+           if (mode < old_mode)
+               mode = old_mode;
+           /* mustn't touch is_update */
+
+           old_infomask |= HEAP_XMAX_INVALID;
+           goto l5;
        }
-       new_xmax = MultiXactIdCreate(xmax, status, add_to_xmax, new_status);
+
+       /* otherwise, just fall back to creating a new multixact */
+       new_status = get_mxact_status_for_lock(mode, is_update);
+       new_xmax = MultiXactIdCreate(xmax, old_status,
+                                    add_to_xmax, new_status);
        GetMultiXactIdHintBits(new_xmax, &new_infomask, &new_infomask2);
    }
    else if (!HEAP_XMAX_IS_LOCKED_ONLY(old_infomask) &&