Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commite0ed20d

Browse files
committed
Prevent access to no-longer-pinned buffer in heapam_tuple_lock().
heap_fetch() used to have a "keep_buf" parameter that told it to returnownership of the buffer pin to the caller after finding that therequested tuple TID exists but is invisible to the specified snapshot.This was thoughtlessly removed in commit5db6df0, which brokeheapam_tuple_lock() (formerly EvalPlanQualFetch) because that functionneeds to do more accesses to the tuple even if it's invisible. The neteffect is that we would continue to touch the page for a microsecond ortwo after releasing pin on the buffer. Usually no harm would result;but if a different session decided to defragment the page concurrently,we could see garbage data and mistakenly conclude that there's no newertuple version to chain up to. (It's hard to say whether this hashappened in the field. The bug was actually found thanks to a laterchange that allowed valgrind to detect accesses to non-pinned buffers.)The most reasonable way to fix this is to reintroduce keep_buf,although I made it behave slightly differently: buffer ownershipis passed back only if there is a valid tuple at the requested TID.In HEAD, we can just add the parameter back to heap_fetch().To avoid an API break in the back branches, introduce an additionalfunction heap_fetch_extended() in those branches.In HEAD there is an additional, less obvious API change: tuple->t_datawill be set to NULL in all cases where buffer ownership is not returned,in particular when the tuple exists but fails the time qual (and!keep_buf). This is to defend against any other callers attempting toaccess non-pinned buffers. We concluded that making that change in backbranches would be more likely to introduce problems than cure any.In passing, remove a comment about heap_fetch that was obsoleted by9a8ee1d.Per bug #17462 from Daniil Anisimov. Back-patch to v12 where the bugwas introduced.Discussion:https://postgr.es/m/17462-9c98a0f00df9bd36@postgresql.org
1 parent2eb3ec8 commite0ed20d

File tree

3 files changed

+43
-15
lines changed

3 files changed

+43
-15
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 33 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1405,10 +1405,13 @@ heap_getnextslot(TableScanDesc sscan, ScanDirection direction, TupleTableSlot *s
14051405
* must unpin the buffer when done with the tuple.
14061406
*
14071407
* If the tuple is not found (ie, item number references a deleted slot),
1408-
* then tuple->t_data is set to NULL and false is returned.
1408+
* then tuple->t_data is set to NULL, *userbuf is set to InvalidBuffer,
1409+
* and false is returned.
14091410
*
14101411
* If the tuple is found but fails the time qual check, then false is returned
1411-
* but tuple->t_data is left pointing to the tuple.
1412+
* and *userbuf is set to InvalidBuffer, but tuple->t_data is left pointing
1413+
* to the tuple. (Note that it is unsafe to dereference tuple->t_data in
1414+
* this case, but callers might choose to test it for NULL-ness.)
14121415
*
14131416
* heap_fetch does not follow HOT chains: only the exact TID requested will
14141417
* be fetched.
@@ -1427,6 +1430,25 @@ heap_fetch(Relation relation,
14271430
Snapshotsnapshot,
14281431
HeapTupletuple,
14291432
Buffer*userbuf)
1433+
{
1434+
returnheap_fetch_extended(relation,snapshot,tuple,userbuf, false);
1435+
}
1436+
1437+
/*
1438+
*heap_fetch_extended- fetch tuple even if it fails snapshot test
1439+
*
1440+
* If keep_buf is true, then upon finding a tuple that is valid but fails
1441+
* the snapshot check, we return the tuple pointer in tuple->t_data and the
1442+
* buffer ID in *userbuf, keeping the buffer pin, just as if it had passed
1443+
* the snapshot. (The function result is still "false" though.)
1444+
* If keep_buf is false then this behaves identically to heap_fetch().
1445+
*/
1446+
bool
1447+
heap_fetch_extended(Relationrelation,
1448+
Snapshotsnapshot,
1449+
HeapTupletuple,
1450+
Buffer*userbuf,
1451+
boolkeep_buf)
14301452
{
14311453
ItemPointertid=&(tuple->t_self);
14321454
ItemIdlp;
@@ -1508,9 +1530,14 @@ heap_fetch(Relation relation,
15081530
return true;
15091531
}
15101532

1511-
/* Tuple failed time qual */
1512-
ReleaseBuffer(buffer);
1513-
*userbuf=InvalidBuffer;
1533+
/* Tuple failed time qual, but maybe caller wants to see it anyway. */
1534+
if (keep_buf)
1535+
*userbuf=buffer;
1536+
else
1537+
{
1538+
ReleaseBuffer(buffer);
1539+
*userbuf=InvalidBuffer;
1540+
}
15141541

15151542
return false;
15161543
}
@@ -1533,8 +1560,7 @@ heap_fetch(Relation relation,
15331560
* are vacuumable, false if not.
15341561
*
15351562
* Unlike heap_fetch, the caller must already have pin and (at least) share
1536-
* lock on the buffer; it is still pinned/locked at exit. Also unlike
1537-
* heap_fetch, we do not report any pgstats count; caller may do so if wanted.
1563+
* lock on the buffer; it is still pinned/locked at exit.
15381564
*/
15391565
bool
15401566
heap_hot_search_buffer(ItemPointertid,Relationrelation,Bufferbuffer,

‎src/backend/access/heap/heapam_handler.c

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,8 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
405405
errmsg("tuple to be locked was already moved to another partition due to concurrent update")));
406406

407407
tuple->t_self=*tid;
408-
if (heap_fetch(relation,&SnapshotDirty,tuple,&buffer))
408+
if (heap_fetch_extended(relation,&SnapshotDirty,tuple,
409+
&buffer, true))
409410
{
410411
/*
411412
* If xmin isn't what we're expecting, the slot must have
@@ -504,6 +505,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
504505
*/
505506
if (tuple->t_data==NULL)
506507
{
508+
Assert(!BufferIsValid(buffer));
507509
returnTM_Deleted;
508510
}
509511

@@ -513,8 +515,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
513515
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple->t_data),
514516
priorXmax))
515517
{
516-
if (BufferIsValid(buffer))
517-
ReleaseBuffer(buffer);
518+
ReleaseBuffer(buffer);
518519
returnTM_Deleted;
519520
}
520521

@@ -530,22 +531,20 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
530531
*
531532
* As above, it should be safe to examine xmax and t_ctid
532533
* without the buffer content lock, because they can't be
533-
* changing.
534+
* changing. We'd better hold a buffer pin though.
534535
*/
535536
if (ItemPointerEquals(&tuple->t_self,&tuple->t_data->t_ctid))
536537
{
537538
/* deleted, so forget about it */
538-
if (BufferIsValid(buffer))
539-
ReleaseBuffer(buffer);
539+
ReleaseBuffer(buffer);
540540
returnTM_Deleted;
541541
}
542542

543543
/* updated, so look at the updated row */
544544
*tid=tuple->t_data->t_ctid;
545545
/* updated row should have xmin matching this xmax */
546546
priorXmax=HeapTupleHeaderGetUpdateXid(tuple->t_data);
547-
if (BufferIsValid(buffer))
548-
ReleaseBuffer(buffer);
547+
ReleaseBuffer(buffer);
549548
/* loop back to fetch next in chain */
550549
}
551550
}

‎src/include/access/heapam.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,9 @@ extern bool heap_getnextslot(TableScanDesc sscan,
123123

124124
externboolheap_fetch(Relationrelation,Snapshotsnapshot,
125125
HeapTupletuple,Buffer*userbuf);
126+
externboolheap_fetch_extended(Relationrelation,Snapshotsnapshot,
127+
HeapTupletuple,Buffer*userbuf,
128+
boolkeep_buf);
126129
externboolheap_hot_search_buffer(ItemPointertid,Relationrelation,
127130
Bufferbuffer,Snapshotsnapshot,HeapTupleheapTuple,
128131
bool*all_dead,boolfirst_call);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp