From: Alvaro Herrera Date: Thu, 19 Dec 2013 19:39:59 +0000 (-0300) Subject: Optimize updating a row that's locked by same xid X-Git-Tag: REL9_3_3~94 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=85d3b3c3ac16ea7d8f7b513d8ac7bc2f88bb988f;p=postgresql 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 70748e5a95..b5977da8f0 100644 --- 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) &&