database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-07-28 23:42:10 +03:00
Code Activity

Fix predicate-locking of HOT updated rows.

Browse Source 
In serializable mode, heap_hot_search_buffer() incorrectly acquired a
predicate lock on the root tuple, not the returned tuple that satisfied
the visibility checks. As explained in README-SSI, the predicate lock does
not need to be copied or extended to other tuple versions, but for that to
work, the correct, visible, tuple version must be locked in the first
place.

The original SSI commit had this bug in it, but it was fixed back in 2013,
in commit 81fbbfe335. But unfortunately, it was reintroduced a few months
later in commit b89e151054. Wising up from that, add a regression test
to cover this, so that it doesn't get reintroduced again. Also, move the
code that sets 't_self', so that it happens at the same time that the
other HeapTuple fields are set, to make it more clear that all the code in
the loop operate on the "current" tuple in the chain, not the root tuple.

Bug spotted by Andres Freund, analysis and original fix by Thomas Munro,
test case and some additional changes to the fix by Heikki Linnakangas.
Backpatch to all supported versions (9.4).

Discussion: https://www.postgresql.org/message-id/20190731210630.nqhszuktygwftjty%40alap3.anarazel.de
This commit is contained in:
Heikki Linnakangas
2019-08-07 12:40:49 +03:00
parent 64579be64a
commit 1169fcf129
4 changed files with 70 additions and 17 deletions
Download Patch File Download Diff File Expand all files Collapse all files

29
src/backend/access/heap/heapam.c
View File

@ -1529,6 +1529,7 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
{
Page dp = (Page) BufferGetPage(buffer);
TransactionId prev_xmax = InvalidTransactionId;
BlockNumber blkno;
OffsetNumber offnum;
bool at_chain_start;
bool valid;
@ -1538,14 +1539,13 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
if (all_dead)
*all_dead = first_call;
Assert(TransactionIdIsValid(RecentGlobalXmin));
Assert(ItemPointerGetBlockNumber(tid) == BufferGetBlockNumber(buffer));
blkno = ItemPointerGetBlockNumber(tid);
offnum = ItemPointerGetOffsetNumber(tid);
at_chain_start = first_call;
skip = !first_call;
heapTuple->t_self = *tid;
Assert(TransactionIdIsValid(RecentGlobalXmin));
Assert(BufferGetBlockNumber(buffer) == blkno);
/* Scan through possible multiple members of HOT-chain */
for (;;)
@ -1573,10 +1573,16 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
break;
}
/*
* Update heapTuple to point to the element of the HOT chain we're
* currently investigating. Having t_self set correctly is important
* because the SSI checks and the *Satisfies routine for historical
* MVCC snapshots need the correct tid to decide about the visibility.
*/
heapTuple->t_data = (HeapTupleHeader) PageGetItem(dp, lp);
heapTuple->t_len = ItemIdGetLength(lp);
heapTuple->t_tableOid = RelationGetRelid(relation);
ItemPointerSetOffsetNumber(&heapTuple->t_self, offnum);
ItemPointerSet(&heapTuple->t_self, blkno, offnum);
/*
* Shouldn't see a HEAP_ONLY tuple at chain start.
@ -1602,21 +1608,10 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
*/
if (!skip)
{
/*
* For the benefit of logical decoding, have t_self point at the
* element of the HOT chain we're currently investigating instead
* of the root tuple of the HOT chain. This is important because
* the *Satisfies routine for historical mvcc snapshots needs the
* correct tid to decide about the visibility in some cases.
*/
ItemPointerSet(&(heapTuple->t_self), BufferGetBlockNumber(buffer), offnum);
/* If it's visible per the snapshot, we must return it */
valid = HeapTupleSatisfiesVisibility(heapTuple, snapshot, buffer);
CheckForSerializableConflictOut(valid, relation, heapTuple,
buffer, snapshot);
/* reset to original, non-redirected, tid */
heapTuple->t_self = *tid;
if (valid)
{
@ -1648,7 +1643,7 @@ heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
if (HeapTupleIsHotUpdated(heapTuple))
{
Assert(ItemPointerGetBlockNumber(&heapTuple->t_data->t_ctid) ==
ItemPointerGetBlockNumber(tid));
blkno);
offnum = ItemPointerGetOffsetNumber(&heapTuple->t_data->t_ctid);
at_chain_start = false;
prev_xmax = HeapTupleHeaderGetUpdateXid(heapTuple->t_data);