From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: anisimow.d@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17462: Invalid memory access in heapam_tuple_lock
Date: Tue, 12 Apr 2022 17:40:05 -0400
Message-ID: <995052.1649799605@sss.pgh.pa.us> (raw)
In-Reply-To: <708918.1649718451@sss.pgh.pa.us>
References: <17462-9c98a0f00df9bd36@postgresql.org>
<286748.1649692519@sss.pgh.pa.us>
<288544.1649693991@sss.pgh.pa.us>
<20220411173417.ocwjn5xrkqqjiysl@alap3.anarazel.de>
<352840.1649699498@sss.pgh.pa.us>
<20220411190042.o5ebdhc5qsejleff@alap3.anarazel.de>
<496872.1649705112@sss.pgh.pa.us>
<20220411194051.sovwssdawxoh4m5h@alap3.anarazel.de>
<530434.1649707170@sss.pgh.pa.us>
<20220411203203.avlrzwy526j4yfas@alap3.anarazel.de>
<708918.1649718451@sss.pgh.pa.us>
I wrote:
> As written here, even though there's no obvious API break, there is
> a subtle change: heap_fetch will now return tuple->t_data = NULL after
> a snapshot failure. I'm of two minds whether to back-patch that part
> (not doing so would require more changes in heapam_tuple_lock, though).
After further thought I think we shouldn't back-patch that part: it
could easily break code that's functionally correct today, and there
isn't any case that it makes better without caller-code changes.
(I'm not sure what made me think it'd affect heapam_tuple_lock; that
function only cares about the behavior with keep_buf = true.)
Hence, I end up with two different patches for HEAD and the back branches.
In HEAD, we explicitly break heap_fetch compatibility by adding a new
argument, and we can make the commit message note the more subtle change
too. In the back branches, the behavior of heap_fetch is unchanged
and heapam_tuple_lock has to use heap_fetch_extended.
regards, tom lane
Attachments:
[text/x-diff] heap_fetch-fix-HEAD.patch (5.5K, ../995052.1649799605@sss.pgh.pa.us/2-heap_fetch-fix-HEAD.patch)
download | inline diff:diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex ba11bcd99e..f6cca1cb1d 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -1530,10 +1530,14 @@ heap_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
* must unpin the buffer when done with the tuple.
*
* If the tuple is not found (ie, item number references a deleted slot),
- * then tuple->t_data is set to NULL and false is returned.+ * then tuple->t_data is set to NULL, *userbuf is set to InvalidBuffer,+ * and false is returned.
*
- * If the tuple is found but fails the time qual check, then false is returned- * but tuple->t_data is left pointing to the tuple.+ * If the tuple is found but fails the time qual check, then the behavior+ * depends on the keep_buf parameter. If keep_buf is false, the results+ * are the same as for the tuple-not-found case. If keep_buf is true,+ * then tuple->t_data and *userbuf are returned as for the success case,+ * and again the caller must unpin the buffer; but false is returned.
*
* heap_fetch does not follow HOT chains: only the exact TID requested will
* be fetched.
@@ -1551,7 +1555,8 @@ bool
heap_fetch(Relation relation,
Snapshot snapshot,
HeapTuple tuple,
- Buffer *userbuf)+ Buffer *userbuf,+ bool keep_buf)
{
ItemPointer tid = &(tuple->t_self);
ItemId lp;
@@ -1634,9 +1639,15 @@ heap_fetch(Relation relation,
return true;
}
- /* Tuple failed time qual */- ReleaseBuffer(buffer);- *userbuf = InvalidBuffer;+ /* Tuple failed time qual, but maybe caller wants to see it anyway. */+ if (keep_buf)+ *userbuf = buffer;+ else+ {+ ReleaseBuffer(buffer);+ *userbuf = InvalidBuffer;+ tuple->t_data = NULL;+ }
return false;
}
@@ -1659,8 +1670,7 @@ heap_fetch(Relation relation,
* are vacuumable, false if not.
*
* Unlike heap_fetch, the caller must already have pin and (at least) share
- * lock on the buffer; it is still pinned/locked at exit. Also unlike- * heap_fetch, we do not report any pgstats count; caller may do so if wanted.+ * lock on the buffer; it is still pinned/locked at exit.
*/
bool
heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
@@ -5379,7 +5389,7 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
block = ItemPointerGetBlockNumber(&tupid);
ItemPointerCopy(&tupid, &(mytup.t_self));
- if (!heap_fetch(rel, SnapshotAny, &mytup, &buf))+ if (!heap_fetch(rel, SnapshotAny, &mytup, &buf, false))
{
/*
* if we fail to find the updated version of the tuple, it's
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.cindex 666b6205a7..444f027149 100644--- a/src/backend/access/heap/heapam_handler.c+++ b/src/backend/access/heap/heapam_handler.c@@ -188,7 +188,7 @@ heapam_fetch_row_version(Relation relation,
Assert(TTS_IS_BUFFERTUPLE(slot));
bslot->base.tupdata.t_self = *tid;
- if (heap_fetch(relation, snapshot, &bslot->base.tupdata, &buffer))+ if (heap_fetch(relation, snapshot, &bslot->base.tupdata, &buffer, false))
{
/* store in slot, transferring existing pin */
ExecStorePinnedBufferHeapTuple(&bslot->base.tupdata, slot, buffer);
@@ -401,7 +401,7 @@ tuple_lock_retry:
errmsg("tuple to be locked was already moved to another partition due to concurrent update")));
tuple->t_self = *tid;
- if (heap_fetch(relation, &SnapshotDirty, tuple, &buffer))+ if (heap_fetch(relation, &SnapshotDirty, tuple, &buffer, true))
{
/*
* If xmin isn't what we're expecting, the slot must have
@@ -500,6 +500,7 @@ tuple_lock_retry:
*/
if (tuple->t_data == NULL)
{
+ Assert(!BufferIsValid(buffer));
return TM_Deleted;
}
@@ -509,8 +510,7 @@ tuple_lock_retry:
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple->t_data),
priorXmax))
{
- if (BufferIsValid(buffer))- ReleaseBuffer(buffer);+ ReleaseBuffer(buffer);
return TM_Deleted;
}
@@ -526,13 +526,12 @@ tuple_lock_retry:
*
* As above, it should be safe to examine xmax and t_ctid
* without the buffer content lock, because they can't be
- * changing.+ * changing. We'd better hold a buffer pin though.
*/
if (ItemPointerEquals(&tuple->t_self, &tuple->t_data->t_ctid))
{
/* deleted, so forget about it */
- if (BufferIsValid(buffer))- ReleaseBuffer(buffer);+ ReleaseBuffer(buffer);
return TM_Deleted;
}
@@ -540,8 +539,7 @@ tuple_lock_retry:
*tid = tuple->t_data->t_ctid;
/* updated row should have xmin matching this xmax */
priorXmax = HeapTupleHeaderGetUpdateXid(tuple->t_data);
- if (BufferIsValid(buffer))- ReleaseBuffer(buffer);+ ReleaseBuffer(buffer);
/* loop back to fetch next in chain */
}
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.hindex 4403f01e13..5d7f7fd800 100644--- a/src/include/access/heapam.h+++ b/src/include/access/heapam.h@@ -133,7 +133,7 @@ extern bool heap_getnextslot_tidrange(TableScanDesc sscan,
ScanDirection direction,
TupleTableSlot *slot);
extern bool heap_fetch(Relation relation, Snapshot snapshot,
- HeapTuple tuple, Buffer *userbuf);+ HeapTuple tuple, Buffer *userbuf, bool keep_buf);
extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
Buffer buffer, Snapshot snapshot, HeapTuple heapTuple,
bool *all_dead, bool first_call);
[text/x-diff] heap_fetch-fix-v14.patch (5.2K, ../995052.1649799605@sss.pgh.pa.us/3-heap_fetch-fix-v14.patch)
download | inline diff:diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.cindex 763dc07361..64b9ec0376 100644--- a/src/backend/access/heap/heapam.c+++ b/src/backend/access/heap/heapam.c@@ -1576,10 +1576,13 @@ heap_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
* must unpin the buffer when done with the tuple.
*
* If the tuple is not found (ie, item number references a deleted slot),
- * then tuple->t_data is set to NULL and false is returned.+ * then tuple->t_data is set to NULL, *userbuf is set to InvalidBuffer,+ * and false is returned.
*
* If the tuple is found but fails the time qual check, then false is returned
- * but tuple->t_data is left pointing to the tuple.+ * and *userbuf is set to InvalidBuffer, but tuple->t_data is left pointing+ * to the tuple. (Note that it is unsafe to dereference tuple->t_data in+ * this case, but callers might choose to test it for NULL-ness.)
*
* heap_fetch does not follow HOT chains: only the exact TID requested will
* be fetched.
@@ -1598,6 +1601,25 @@ heap_fetch(Relation relation,
Snapshot snapshot,
HeapTuple tuple,
Buffer *userbuf)
+{+ return heap_fetch_extended(relation, snapshot, tuple, userbuf, false);+}++/*+ * heap_fetch_extended - fetch tuple even if it fails snapshot test+ *+ * If keep_buf is true, then upon finding a tuple that is valid but fails+ * the snapshot check, we return the tuple pointer in tuple->t_data and the+ * buffer ID in *userbuf, keeping the buffer pin, just as if it had passed+ * the snapshot. (The function result is still "false" though.)+ * If keep_buf is false then this behaves identically to heap_fetch().+ */+bool+heap_fetch_extended(Relation relation,+ Snapshot snapshot,+ HeapTuple tuple,+ Buffer *userbuf,+ bool keep_buf)
{
ItemPointer tid = &(tuple->t_self);
ItemId lp;
@@ -1680,9 +1702,14 @@ heap_fetch(Relation relation,
return true;
}
- /* Tuple failed time qual */- ReleaseBuffer(buffer);- *userbuf = InvalidBuffer;+ /* Tuple failed time qual, but maybe caller wants to see it anyway. */+ if (keep_buf)+ *userbuf = buffer;+ else+ {+ ReleaseBuffer(buffer);+ *userbuf = InvalidBuffer;+ }
return false;
}
@@ -1705,8 +1732,7 @@ heap_fetch(Relation relation,
* are vacuumable, false if not.
*
* Unlike heap_fetch, the caller must already have pin and (at least) share
- * lock on the buffer; it is still pinned/locked at exit. Also unlike- * heap_fetch, we do not report any pgstats count; caller may do so if wanted.+ * lock on the buffer; it is still pinned/locked at exit.
*/
bool
heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.cindex d1192e6a0c..66339392d7 100644--- a/src/backend/access/heap/heapam_handler.c+++ b/src/backend/access/heap/heapam_handler.c@@ -401,7 +401,8 @@ tuple_lock_retry:
errmsg("tuple to be locked was already moved to another partition due to concurrent update")));
tuple->t_self = *tid;
- if (heap_fetch(relation, &SnapshotDirty, tuple, &buffer))+ if (heap_fetch_extended(relation, &SnapshotDirty, tuple,+ &buffer, true))
{
/*
* If xmin isn't what we're expecting, the slot must have
@@ -500,6 +501,7 @@ tuple_lock_retry:
*/
if (tuple->t_data == NULL)
{
+ Assert(!BufferIsValid(buffer));
return TM_Deleted;
}
@@ -509,8 +511,7 @@ tuple_lock_retry:
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple->t_data),
priorXmax))
{
- if (BufferIsValid(buffer))- ReleaseBuffer(buffer);+ ReleaseBuffer(buffer);
return TM_Deleted;
}
@@ -526,13 +527,12 @@ tuple_lock_retry:
*
* As above, it should be safe to examine xmax and t_ctid
* without the buffer content lock, because they can't be
- * changing.+ * changing. We'd better hold a buffer pin though.
*/
if (ItemPointerEquals(&tuple->t_self, &tuple->t_data->t_ctid))
{
/* deleted, so forget about it */
- if (BufferIsValid(buffer))- ReleaseBuffer(buffer);+ ReleaseBuffer(buffer);
return TM_Deleted;
}
@@ -540,8 +540,7 @@ tuple_lock_retry:
*tid = tuple->t_data->t_ctid;
/* updated row should have xmin matching this xmax */
priorXmax = HeapTupleHeaderGetUpdateXid(tuple->t_data);
- if (BufferIsValid(buffer))- ReleaseBuffer(buffer);+ ReleaseBuffer(buffer);
/* loop back to fetch next in chain */
}
}
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.hindex e63b49fc38..4f1dff9ca1 100644--- a/src/include/access/heapam.h+++ b/src/include/access/heapam.h@@ -134,6 +134,9 @@ extern bool heap_getnextslot_tidrange(TableScanDesc sscan,
TupleTableSlot *slot);
extern bool heap_fetch(Relation relation, Snapshot snapshot,
HeapTuple tuple, Buffer *userbuf);
+extern bool heap_fetch_extended(Relation relation, Snapshot snapshot,+ HeapTuple tuple, Buffer *userbuf,+ bool keep_buf);
extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
Buffer buffer, Snapshot snapshot, HeapTuple heapTuple,
bool *all_dead, bool first_call);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, anisimow.d@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17462: Invalid memory access in heapam_tuple_lock
In-Reply-To: <995052.1649799605@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox