From: Heikki Linnakangas Date: Wed, 7 Aug 2019 09:40:49 +0000 (+0300) Subject: Fix predicate-locking of HOT updated rows. X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1169fcf129f41762fa8a2d82b52eabd788b3a4de;p=postgresql Fix predicate-locking of HOT updated rows. 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 --- diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index e33f019939..6b42bdc77b 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -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); diff --git a/src/test/isolation/expected/predicate-lock-hot-tuple.out b/src/test/isolation/expected/predicate-lock-hot-tuple.out new file mode 100644 index 0000000000..d1c69bbbd0 --- /dev/null +++ b/src/test/isolation/expected/predicate-lock-hot-tuple.out @@ -0,0 +1,20 @@ +Parsed test spec with 2 sessions + +starting permutation: b1 b2 r1 r2 w1 w2 c1 c2 +step b1: BEGIN ISOLATION LEVEL SERIALIZABLE; +step b2: BEGIN ISOLATION LEVEL SERIALIZABLE; +step r1: SELECT * FROM test WHERE i IN (5, 7) +i t + +5 apple +7 pear_hot_updated +step r2: SELECT * FROM test WHERE i IN (5, 7) +i t + +5 apple +7 pear_hot_updated +step w1: UPDATE test SET t = 'pear_xact1' WHERE i = 7 +step w2: UPDATE test SET t = 'apple_xact2' WHERE i = 5 +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 009c2ec54b..69ae227953 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -17,6 +17,7 @@ test: partial-index test: two-ids test: multiple-row-versions test: index-only-scan +test: predicate-lock-hot-tuple test: deadlock-simple test: deadlock-hard test: deadlock-soft diff --git a/src/test/isolation/specs/predicate-lock-hot-tuple.spec b/src/test/isolation/specs/predicate-lock-hot-tuple.spec new file mode 100644 index 0000000000..d16fb60533 --- /dev/null +++ b/src/test/isolation/specs/predicate-lock-hot-tuple.spec @@ -0,0 +1,37 @@ +# Test predicate locks on HOT updated tuples. +# +# This test has two serializable transactions. Both select two rows +# from the table, and then update one of them. +# If these were serialized (run one at a time), the transaction that +# runs later would see one of the rows to be updated. +# +# Any overlap between the transactions must cause a serialization failure. +# We used to have a bug in predicate locking HOT updated tuples, which +# caused the conflict to be missed, if the row was HOT updated. + +setup +{ + CREATE TABLE test (i int PRIMARY KEY, t text); + INSERT INTO test VALUES (5, 'apple'), (7, 'pear'), (11, 'banana'); + -- HOT-update 'pear' row. + UPDATE test SET t = 'pear_hot_updated' WHERE i = 7; +} + +teardown +{ + DROP TABLE test; +} + +session "s1" +step "b1" { BEGIN ISOLATION LEVEL SERIALIZABLE; } +step "r1" { SELECT * FROM test WHERE i IN (5, 7) } +step "w1" { UPDATE test SET t = 'pear_xact1' WHERE i = 7 } +step "c1" { COMMIT; } + +session "s2" +step "b2" { BEGIN ISOLATION LEVEL SERIALIZABLE; } +step "r2" { SELECT * FROM test WHERE i IN (5, 7) } +step "w2" { UPDATE test SET t = 'apple_xact2' WHERE i = 5 } +step "c2" { COMMIT; } + +permutation "b1" "b2" "r1" "r2" "w1" "w2" "c1" "c2"