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 70748e5a955cf11662384e0f104c27717ccdbf0f..b5977da8f0f233ef5cfdfcf90de60840f62604f9 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -4534,6 +4534,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;
@@ -4541,6 +4543,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)
                {
@@ -4666,21 +4673,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
                        {
@@ -4700,43 +4708,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) &&