Re: An out-of-date comment in nodeIndexonlyscan.c
> On 9 Nov 2021, at 15:28, Daniel Gustafsson wrote: > > This patch now fails to apply, probably due to a mostly trivial collision with > fdd88571454e2b00dbe446e8609c6e4294ca89ae in the test files. Can you submit a > rebased version? I've marked this Returned with Feedback, please feel free to resubmit when a new version of the patch is available. -- Daniel Gustafsson https://vmware.com/
Re: An out-of-date comment in nodeIndexonlyscan.c
This patch now fails to apply, probably due to a mostly trivial collision with fdd88571454e2b00dbe446e8609c6e4294ca89ae in the test files. Can you submit a rebased version? -- Daniel Gustafsson https://vmware.com/
Re: An out-of-date comment in nodeIndexonlyscan.c
On Mon, Jun 14, 2021 at 2:29 PM David Rowley wrote: > Have you also thought about deferrable unique / primary key constraints? > > It's possible to the uniqueness temporarily violated during a > transaction when the unique constraint is deferred, Oh, yeah, very good question. I think many scenarios involving duplicates work correctly, but I think there is a danger like this: create table t (i int primary key deferrable initially deferred, j int); create unique index on t(j); insert into t values (999, 999); -- avoid empty index set enable_seqscan = off; begin transaction isolation level serializable; insert into t values (1, 2); -- create phantom row select * from t where i = 1; delete from t where j = 2; -- remove phantom row SELECT locktype, relation::regclass, page, tuple FROM pg_locks WHERE mode = 'SIReadLock'; commit; master: locktype | relation | page | tuple --+--+--+--- page | t_pkey |1 | page | t_j_idx |1 | (2 rows) v3 patch: locktype | relation | page | tuple --+--+--+--- (0 rows) In fact the lock on t_pkey's page 1 was important: it represents the search for a tuple with i = 1, other than our temporary phantom (only allowed because constraint deferred). If someone else inserts a row matching i = 1, the SSI system will not know that we tried to look for it, because our own temporary phantom confused us. > I think you'd just need to add a check to ensure that indimmediate is > true around where you're checking the indisunique flag. Yeah, that fixes the problem in this case at least. With v4: locktype | relation | page | tuple --+--+--+--- page | t_pkey |1 | (1 row) (It's expected that t_j_idx is not locked: the delete query benefits from the optimisation when accessing the index on t(j)). That test case is a little confusing, because at no point does it ever actually create a duplicate, but I suspect the problem is avoided without the deferred constraint because you'd either get a UCV on insertion, or block anyone else from inserting until after you commit (even after you delete the phantom), and I think that may avoid the hazard. I could be confused about that. If I am wrong, then a possible general solution may be to apply the optimisation only if we find a match that wasn't written by this transaction, though I'm not sure how given the current layering. From e52860f760ade79813aae7beb7e9d81ab0d03f34 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 10 Jun 2021 23:35:16 +1200 Subject: [PATCH v4 1/2] 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 00..fef5b8d398 --- /dev/null +++ b/src/test/isolation/expected/index-only-scan-2.out @@
Re: An out-of-date comment in nodeIndexonlyscan.c
On Mon, 14 Jun 2021 at 10:03, Thomas Munro wrote: > > On Mon, Jun 14, 2021 at 12:54 AM David Rowley wrote: > > I think a more optimal and nicer way of doing that would be setting > > bits in a Bitmapset then checking bms_num_members is equal to > > n_scan_keys. > > Shouldn't it be compared with indnkeyatts? Yes, much nicer, thanks! Oh yeah, I did mean that. Thanks for the correction. Have you also thought about deferrable unique / primary key constraints? It's possible to the uniqueness temporarily violated during a transaction when the unique constraint is deferred, For example: create table t (id int primary key deferrable initially deferred); begin; insert into t values(1),(1); select * from t; id 1 1 (2 rows) I think you'd just need to add a check to ensure that indimmediate is true around where you're checking the indisunique flag. David
Re: An out-of-date comment in nodeIndexonlyscan.c
On Mon, Jun 14, 2021 at 12:54 AM David Rowley wrote: > I think a more optimal and nicer way of doing that would be setting > bits in a Bitmapset then checking bms_num_members is equal to > n_scan_keys. Shouldn't it be compared with indnkeyatts? Yes, much nicer, thanks! From 2325ae403196f73865f7c6b3224db925ca406969 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 10 Jun 2021 23:35:16 +1200 Subject: [PATCH v3 1/2] 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 00..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 00..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 00..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
Re: An out-of-date comment in nodeIndexonlyscan.c
On Mon, 14 Jun 2021 at 00:02, Thomas Munro wrote:
> Here's a highly experimental patch I came up with that seems to
> produce the right results in simple cases, without (yet) involving the
> planner.
+ /* Find all equality quals. */
+ for (int i = 0; i < n_scan_keys; ++i)
+ {
+ if (scan_keys[i].sk_strategy == BTEqualStrategyNumber)
+ attnos[nattnos++] = scan_keys[i].sk_attno;
+ }
+
+ /* Are all attributes covered? */
+ /* XXX is this check enough or do we need to work harder? */
+ qsort(attnos, nattnos, sizeof(AttrNumber), compare_int16);
+ nattnos = qunique(attnos, nattnos, sizeof(AttrNumber), compare_int16);
+ if (nattnos == index->rd_index->indnkeyatts)
I think a more optimal and nicer way of doing that would be setting
bits in a Bitmapset then checking bms_num_members is equal to
n_scan_keys.
David
Re: An out-of-date comment in nodeIndexonlyscan.c
On Sat, Jun 12, 2021 at 2:35 PM Thomas Munro wrote: > ... 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. Here's a highly experimental patch I came up with that seems to produce the right results in simple cases, without (yet) involving the planner. The regression tests show single table queries, but it works also for nest loop joins, which is where this optimisation should be most interesting, I think. There are a few weird things about this patch though, and there could well be much better ways to do it, as noted in the commit message and comments. It's a start on the problem... From 2325ae403196f73865f7c6b3224db925ca406969 Mon Sep 17 00:00:00 2001 From: Thomas Munro Date: Thu, 10 Jun 2021 23:35:16 +1200 Subject: [PATCH v2 1/2] 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 00..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 00..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
Re: An out-of-date comment in nodeIndexonlyscan.c
On Fri, Jun 28, 2019 at 12:55 PM Ashwin Agrawal wrote: > On Thu, Jun 27, 2019 at 4:33 PM Thomas Munro 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 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 00..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 00..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 00..cd3c599af8 --- /dev/null +++ b/src/test/isolation/specs/index-only-scan-2.spec @@ -0,0 +1,39 @@ +# Test the
Re: An out-of-date comment in nodeIndexonlyscan.c
On Fri, Jun 28, 2019 at 12:55 PM Ashwin Agrawal wrote: > On Thu, Jun 27, 2019 at 4:33 PM Thomas Munro wrote: >> I wonder if it might be possible to avoid page locks on both the heap >> *and* index in some limited cases, as I mentioned over here (just an >> idea, could be way off base): >> >> https://www.postgresql.org/message-id/CA%2BhUKGJGDVfhHmoUGzi-_J%2BN8FmRjmWTY0MCd1ZV5Fj9qdyb1w%40mail.gmail.com > > I am in same boat as you. One can get serializable conflict error today based > on tuple gets HOT updated or not. HOT is logically internal code optimization > and not so much user functionality, so ideally feels shouldn't affect > serializable behavior. But it does currently, again, due to index locking. > Disable HOT update and 4 isolation tests fail due to "could not serialize > access due to read/write dependencies among transactions" otherwise not. If > the tuple happens to fit on same page user will not get the error, if the > tuple gets inserted to different page the error can happen, due to index page > locking. I had discussed this with Heikki and thinking is we shouldn't need > to take the lock on the index page, if the index key was not changed, even if > it was a non-HOT update. Not sure of complications and implications, but just > a thought. Oh, I think the idea I was suggesting might be the same as this item from README-SSI (unrelated to HOT, but related to index uniqueness, particularly in index-only-scan where you might be able to skip the btree page lock): "Several optimizations are possible, though not all are implemented yet: ... * An index scan which is comparing for equality on the entire key for a unique index need not acquire a predicate lock as long as a key is found corresponding to a visible tuple which has not been modified by another transaction -- there are no "between or around" gaps to cover." -- Thomas Munro https://enterprisedb.com
Re: An out-of-date comment in nodeIndexonlyscan.c
On Fri, Jun 28, 2019 at 12:55 PM Ashwin Agrawal wrote: > Change looks good to me. Pushed, thanks. -- Thomas Munro https://enterprisedb.com
Re: An out-of-date comment in nodeIndexonlyscan.c
On Thu, Jun 27, 2019 at 4:33 PM Thomas Munro 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. > I wonder if it might be possible to avoid page locks on both the heap > *and* index in some limited cases, as I mentioned over here (just an > idea, could be way off base): > > > https://www.postgresql.org/message-id/CA%2BhUKGJGDVfhHmoUGzi-_J%2BN8FmRjmWTY0MCd1ZV5Fj9qdyb1w%40mail.gmail.com I am in same boat as you. One can get serializable conflict error today based on tuple gets HOT updated or not. HOT is logically internal code optimization and not so much user functionality, so ideally feels shouldn't affect serializable behavior. But it does currently, again, due to index locking. Disable HOT update and 4 isolation tests fail due to "could not serialize access due to read/write dependencies among transactions" otherwise not. If the tuple happens to fit on same page user will not get the error, if the tuple gets inserted to different page the error can happen, due to index page locking. I had discussed this with Heikki and thinking is we shouldn't need to take the lock on the index page, if the index key was not changed, even if it was a non-HOT update. Not sure of complications and implications, but just a thought.
Re: An out-of-date comment in nodeIndexonlyscan.c
On Fri, Feb 8, 2019 at 4:58 AM Thomas Munro wrote: > On Thu, May 17, 2018 at 3:49 AM Heikki Linnakangas wrote: > > On 14/05/18 02:15, Thomas Munro wrote: > > > The first sentence of that comment is no longer true as of commit > > > c01262a8 (2013). As for whether it's necessary to predicate-lock the > > > whole eheap page (rather than the heap tuple) anyway because of HOT > > > update chains, I don't know, so I'm not sure what wording to propose > > > instead. > > > > Hmm. If there are any updated tuples, HOT or not, the visibility map bit > > will not be set, and we won't reach this code. So I think we could > > acquire the tuple lock here. > > Right. CC Kevin. I think we should at least fix the comment. As for > changing the lock level, PredicateLockTuple() wants a heap tuple that > we don't have, so we'd probably need to add a PredicateLockTid() > function. 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. I wonder if it might be possible to avoid page locks on both the heap *and* index in some limited cases, as I mentioned over here (just an idea, could be way off base): https://www.postgresql.org/message-id/CA%2BhUKGJGDVfhHmoUGzi-_J%2BN8FmRjmWTY0MCd1ZV5Fj9qdyb1w%40mail.gmail.com -- Thomas Munro https://enterprisedb.com 0001-Fix-misleading-comment-in-nodeIndexonlyscan.c.patch Description: Binary data
Re: An out-of-date comment in nodeIndexonlyscan.c
On Thu, May 17, 2018 at 3:49 AM Heikki Linnakangas wrote: > On 14/05/18 02:15, Thomas Munro wrote: > > Since commit cdf91edb (2012), nodeIndexonlyscan.c says: > > > > /* > > * Predicate locks for index-only scans must be > > acquired at the page > > * level when the heap is not accessed, since > > tuple-level predicate > > * locks need the tuple's xmin value. If we had to > > visit the tuple > > * anyway, then we already have the tuple-level lock > > and can skip the > > * page lock. > > */ > > if (tuple == NULL) > > PredicateLockPage(scandesc->heapRelation, > > > > ItemPointerGetBlockNumber(tid), > > > > estate->es_snapshot); > > > > The first sentence of that comment is no longer true as of commit > > c01262a8 (2013). As for whether it's necessary to predicate-lock the > > whole eheap page (rather than the heap tuple) anyway because of HOT > > update chains, I don't know, so I'm not sure what wording to propose > > instead. > > Hmm. If there are any updated tuples, HOT or not, the visibility map bit > will not be set, and we won't reach this code. So I think we could > acquire the tuple lock here. Right. CC Kevin. I think we should at least fix the comment. As for changing the lock level, PredicateLockTuple() wants a heap tuple that we don't have, so we'd probably need to add a PredicateLockTid() function. -- Thomas Munro http://www.enterprisedb.com
Re: An out-of-date comment in nodeIndexonlyscan.c
On 14/05/18 02:15, Thomas Munro wrote: Hello, Since commit cdf91edb (2012), nodeIndexonlyscan.c says: /* * Predicate locks for index-only scans must be acquired at the page * level when the heap is not accessed, since tuple-level predicate * locks need the tuple's xmin value. If we had to visit the tuple * anyway, then we already have the tuple-level lock and can skip the * page lock. */ if (tuple == NULL) PredicateLockPage(scandesc->heapRelation, ItemPointerGetBlockNumber(tid), estate->es_snapshot); The first sentence of that comment is no longer true as of commit c01262a8 (2013). As for whether it's necessary to predicate-lock the whole eheap page (rather than the heap tuple) anyway because of HOT update chains, I don't know, so I'm not sure what wording to propose instead. Hmm. If there are any updated tuples, HOT or not, the visibility map bit will not be set, and we won't reach this code. So I think we could acquire the tuple lock here. - Heikki
An out-of-date comment in nodeIndexonlyscan.c
Hello, Since commit cdf91edb (2012), nodeIndexonlyscan.c says: /* * Predicate locks for index-only scans must be acquired at the page * level when the heap is not accessed, since tuple-level predicate * locks need the tuple's xmin value. If we had to visit the tuple * anyway, then we already have the tuple-level lock and can skip the * page lock. */ if (tuple == NULL) PredicateLockPage(scandesc->heapRelation, ItemPointerGetBlockNumber(tid), estate->es_snapshot); The first sentence of that comment is no longer true as of commit c01262a8 (2013). As for whether it's necessary to predicate-lock the whole eheap page (rather than the heap tuple) anyway because of HOT update chains, I don't know, so I'm not sure what wording to propose instead. -- Thomas Munro http://www.enterprisedb.com
