On Fri, Jun 28, 2019 at 12:55 PM Ashwin Agrawal <aagra...@pivotal.io> wrote: > On Thu, Jun 27, 2019 at 4:33 PM Thomas Munro <thomas.mu...@gmail.com> wrote: >> >> Here's a patch I'd like to commit to fix the comment. We could look >> at improving the actual code after >> https://commitfest.postgresql.org/23/2169/ is done. > > Change looks good to me.
... and here is the corresponding code change, with a test to demonstrate the change. I'm working on a proof-of-concept to skip the btree page lock sometimes too, which seems promising, but it requires some planner work which has temporarily pretzeled my brain.
From 1039ec4ffbb9da15812a51f4eb9e8be32520fa63 Mon Sep 17 00:00:00 2001 From: Thomas Munro <thomas.mu...@gmail.com> Date: Thu, 10 Jun 2021 23:35:16 +1200 Subject: [PATCH] Use tuple-level SIREAD locks for index-only scans. When index-only scans manage to avoid fetching heap tuples, previously we'd predicate-lock the whole heap page (since commit cdf91edb). Commits c01262a8 and 6f38d4da made it possible to lock the tuple instead with only a TID, to match the behavior of regular index scans and avoid some false conflicts. Discussion: https://postgr.es/m/CAEepm%3D2GK3FVdnt5V3d%2Bh9njWipCv_fNL%3DwjxyUhzsF%3D0PcbNg%40mail.gmail.com --- src/backend/executor/nodeIndexonlyscan.c | 12 ++++-- .../isolation/expected/index-only-scan-2.out | 15 +++++++ .../isolation/expected/index-only-scan-3.out | 16 ++++++++ src/test/isolation/isolation_schedule | 2 + .../isolation/specs/index-only-scan-2.spec | 39 +++++++++++++++++++ .../isolation/specs/index-only-scan-3.spec | 33 ++++++++++++++++ 6 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 src/test/isolation/expected/index-only-scan-2.out create mode 100644 src/test/isolation/expected/index-only-scan-3.out create mode 100644 src/test/isolation/specs/index-only-scan-2.spec create mode 100644 src/test/isolation/specs/index-only-scan-3.spec diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index 0754e28a9a..f7185b4519 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -243,12 +243,16 @@ IndexOnlyNext(IndexOnlyScanState *node) /* * If we didn't access the heap, then we'll need to take a predicate - * lock explicitly, as if we had. For now we do that at page level. + * lock explicitly, as if we had. We don't have the inserter's xid, + * but that's only used by PredicateLockTID to check if it matches the + * caller's top level xid, and it can't possibly have been inserted by + * us or the page wouldn't be all visible. */ if (!tuple_from_heap) - PredicateLockPage(scandesc->heapRelation, - ItemPointerGetBlockNumber(tid), - estate->es_snapshot); + PredicateLockTID(scandesc->heapRelation, + tid, + estate->es_snapshot, + InvalidTransactionId); return slot; } diff --git a/src/test/isolation/expected/index-only-scan-2.out b/src/test/isolation/expected/index-only-scan-2.out new file mode 100644 index 0000000000..fef5b8d398 --- /dev/null +++ b/src/test/isolation/expected/index-only-scan-2.out @@ -0,0 +1,15 @@ +Parsed test spec with 2 sessions + +starting permutation: r1 r2 w1 w2 c1 c2 +step r1: SELECT id1 FROM account WHERE id1 = 1; +id1 + +1 +step r2: SELECT id2 FROM account WHERE id2 = 2; +id2 + +2 +step w1: UPDATE account SET balance = 200 WHERE id1 = 1; +step w2: UPDATE account SET balance = 200 WHERE id2 = 2; +step c1: COMMIT; +step c2: COMMIT; diff --git a/src/test/isolation/expected/index-only-scan-3.out b/src/test/isolation/expected/index-only-scan-3.out new file mode 100644 index 0000000000..efef162779 --- /dev/null +++ b/src/test/isolation/expected/index-only-scan-3.out @@ -0,0 +1,16 @@ +Parsed test spec with 2 sessions + +starting permutation: r1 r2 w1 w2 c1 c2 +step r1: SELECT id1 FROM account WHERE id1 = 2; +id1 + +2 +step r2: SELECT id2 FROM account WHERE id2 = 1; +id2 + +1 +step w1: UPDATE account SET balance = 200 WHERE id1 = 1; +step w2: UPDATE account SET balance = 200 WHERE id2 = 2; +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 f4c01006fc..570aedb900 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -17,6 +17,8 @@ test: partial-index test: two-ids test: multiple-row-versions test: index-only-scan +test: index-only-scan-2 +test: index-only-scan-3 test: predicate-lock-hot-tuple test: update-conflict-out test: deadlock-simple diff --git a/src/test/isolation/specs/index-only-scan-2.spec b/src/test/isolation/specs/index-only-scan-2.spec new file mode 100644 index 0000000000..cd3c599af8 --- /dev/null +++ b/src/test/isolation/specs/index-only-scan-2.spec @@ -0,0 +1,39 @@ +# Test the granularity of predicate locks on heap tuples, for index-only scans. + +# Access the accounts through two different indexes, so that index predicate +# locks don't confuse matters; here we only want to test heap locking. Since +# s1 and s2 access different heap tuples there is no cycle, so both +# transactions should be able to commit. Before PostgreSQL 15, a serialization +# failure was reported because of a page-level SIREAD lock on the heap that +# produced a false conflict. + +setup +{ + CREATE TABLE account (id1 int, id2 int, balance int); + CREATE UNIQUE INDEX ON account(id1); + CREATE UNIQUE INDEX ON account(id2); + INSERT INTO account VALUES (1, 1, 100), (2, 2, 100); +} +setup +{ + VACUUM account; +} + +teardown +{ + DROP TABLE account; +} + +session "s1" +setup { BEGIN ISOLATION LEVEL SERIALIZABLE; SET enable_seqscan = off; } +step "r1" { SELECT id1 FROM account WHERE id1 = 1; } +step "w1" { UPDATE account SET balance = 200 WHERE id1 = 1; } +step "c1" { COMMIT; } + +session "s2" +setup { BEGIN ISOLATION LEVEL SERIALIZABLE; SET enable_seqscan = off; } +step "r2" { SELECT id2 FROM account WHERE id2 = 2; } +step "w2" { UPDATE account SET balance = 200 WHERE id2 = 2; } +step "c2" { COMMIT; } + +permutation "r1" "r2" "w1" "w2" "c1" "c2" diff --git a/src/test/isolation/specs/index-only-scan-3.spec b/src/test/isolation/specs/index-only-scan-3.spec new file mode 100644 index 0000000000..5f04923002 --- /dev/null +++ b/src/test/isolation/specs/index-only-scan-3.spec @@ -0,0 +1,33 @@ +# A variant of index-only-scan-2.spec that is a true conflict, detected via +# heap tuple locks only. + +setup +{ + CREATE TABLE account (id1 int, id2 int, balance int); + CREATE UNIQUE INDEX ON account(id1); + CREATE UNIQUE INDEX ON account(id2); + INSERT INTO account VALUES (1, 1, 100), (2, 2, 100); +} +setup +{ + VACUUM account; +} + +teardown +{ + DROP TABLE account; +} + +session "s1" +setup { BEGIN ISOLATION LEVEL SERIALIZABLE; SET enable_seqscan = off; } +step "r1" { SELECT id1 FROM account WHERE id1 = 2; } +step "w1" { UPDATE account SET balance = 200 WHERE id1 = 1; } +step "c1" { COMMIT; } + +session "s2" +setup { BEGIN ISOLATION LEVEL SERIALIZABLE; SET enable_seqscan = off; } +step "r2" { SELECT id2 FROM account WHERE id2 = 1; } +step "w2" { UPDATE account SET balance = 200 WHERE id2 = 2; } +step "c2" { COMMIT; } + +permutation "r1" "r2" "w1" "w2" "c1" "c2" -- 2.30.2
