From cab72f0fd08cceb975194d2acd2f6eed917e8fb8 Mon Sep 17 00:00:00 2001 From: Jeff Davis Date: Thu, 22 Sep 2022 10:58:49 -0700 Subject: [PATCH] Fix race condition where heap_delete() fails to pin VM page. Similar to 5f12bc94dc, the code must re-check PageIsAllVisible() after buffer lock is re-acquired. Backpatching to the same version, 12. Discussion: https://postgr.es/m/CAEP4nAw9jYQDKd_5Y+-s2E4YiUJq1vqiikFjYGpLShtp-K3gag@mail.gmail.com Reported-by: Robins Tharakan Reviewed-by: Robins Tharakan Backpatch-through: 12 --- src/backend/access/heap/heapam.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 98669c51c9e..dc90660fe3c 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -2531,6 +2531,15 @@ heap_delete(Relation relation, ItemPointer tid, LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); + lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid)); + Assert(ItemIdIsNormal(lp)); + + tp.t_tableOid = RelationGetRelid(relation); + tp.t_data = (HeapTupleHeader) PageGetItem(page, lp); + tp.t_len = ItemIdGetLength(lp); + tp.t_self = *tid; + +l1: /* * If we didn't pin the visibility map page and the page has become all * visible while we were busy locking the buffer, we'll have to unlock and @@ -2544,15 +2553,6 @@ heap_delete(Relation relation, ItemPointer tid, LockBuffer(buffer, BUFFER_LOCK_EXCLUSIVE); } - lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid)); - Assert(ItemIdIsNormal(lp)); - - tp.t_tableOid = RelationGetRelid(relation); - tp.t_data = (HeapTupleHeader) PageGetItem(page, lp); - tp.t_len = ItemIdGetLength(lp); - tp.t_self = *tid; - -l1: result = HeapTupleSatisfiesUpdate(&tp, cid, buffer); if (result == TM_Invisible) @@ -2611,8 +2611,12 @@ l1: * If xwait had just locked the tuple then some other xact * could update this tuple before we get to this point. Check * for xmax change, and start over if so. + * + * We also must start over if we didn't pin the VM page, and + * the page has become all visible. */ - if (xmax_infomask_changed(tp.t_data->t_infomask, infomask) || + if ((vmbuffer == InvalidBuffer && PageIsAllVisible(page)) || + xmax_infomask_changed(tp.t_data->t_infomask, infomask) || !TransactionIdEquals(HeapTupleHeaderGetRawXmax(tp.t_data), xwait)) goto l1; @@ -2644,8 +2648,12 @@ l1: * xwait is done, but if xwait had just locked the tuple then some * other xact could update this tuple before we get to this point. * Check for xmax change, and start over if so. + * + * We also must start over if we didn't pin the VM page, and the + * page has become all visible. */ - if (xmax_infomask_changed(tp.t_data->t_infomask, infomask) || + if ((vmbuffer == InvalidBuffer && PageIsAllVisible(page)) || + xmax_infomask_changed(tp.t_data->t_infomask, infomask) || !TransactionIdEquals(HeapTupleHeaderGetRawXmax(tp.t_data), xwait)) goto l1;