From: David Christensen <david@endpoint.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16676: SELECT ... FETCH FIRST ROW WITH TIES FOR UPDATE SKIP LOCKED locks rows it doesn't return
Date: Tue, 20 Oct 2020 11:18:20 -0500
Message-ID: <C0CF9D56-0392-43D2-8577-5F8A01AF2B56@endpoint.com> (raw)
In-Reply-To: <05226762-2C0B-44DE-A584-D9FCCFF06B5F@endpoint.com>
References: <20201020134114.GA11045@alvherre.pgsql>
<604509.1603203416@sss.pgh.pa.us>
<05226762-2C0B-44DE-A584-D9FCCFF06B5F@endpoint.com>
> On Oct 20, 2020, at 9:23 AM, David Christensen <david@endpoint.com> wrote:
>
>> On Oct 20, 2020, at 9:16 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>>> On 2020-Oct-19, David Christensen wrote:
>>>> Maybe splitting LockRows into two nodes, one for locking and one for
>>>> emitting unlocked nodes then interleaving Limit in between? (Or only
>>>> doing something along these lines for this admittedly narrow use case.)
>>
>>> I was having a similar idea, but the main reason I don't think it's a
>>> good fix is that we can't backpatch such a change to pg13.
>>
>> Um, why not? I don't have a position yet on whether this is a good way
>> to fix it; but if we did do it, AFAICS the only thing we'd have to be
>> careful of in v13 is not renumbering existing NodeTag values.
>>
>> If your concern is just that EXPLAIN plans will look different, the same
>> could be said of David's other proposal.
>
> If we can determine the scope to which these different nodes might be used and make it so it still uses the existing node structure for most existing cases (i.e., maybe an additional parameter to create_lockrows_plan or similar) then that might work; AFAIK, this is the first time we’ve had to consider this sort of thing (though I do wonder if there might be something in window functions which might bump into this sort of issue).
>
> I’m working on an isolation test to formalize this failure, which I can do as a separate patch if we want to have this prior to implementing a fix.
Enclosed is the (currently failing) isolation test for the expected behavior.
--
David Christensen
Senior Software and Database Engineer
End Point Corporation
david@endpoint.com
785-727-1171
Attachments:0001-Add-isolation-test-for-FETCH-FIRST-ROW-WITH-TIES-FOR.patch (application/octet-stream, 2.7K) download
From ce874d9dd8afee20e5bd547b9cc17d1803168aa0 Mon Sep 17 00:00:00 2001
From: David Christensen <david@endpoint.com>
Date: Tue, 20 Oct 2020 10:17:58 -0500
Subject: [PATCH] Add isolation test for FETCH FIRST ROW WITH TIES FOR UPDATE
SKIP LOCKED
---
src/test/isolation/expected/skip-locked-5.out | 14 +++++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/skip-locked-5.spec | 29 +++++++++++++++++++
3 files changed, 44 insertions(+)
create mode 100644 src/test/isolation/expected/skip-locked-5.out
create mode 100644 src/test/isolation/specs/skip-locked-5.spec
diff --git a/src/test/isolation/expected/skip-locked-5.out b/src/test/isolation/expected/skip-locked-5.outnew file mode 100644index 0000000000..87fc2d8aa1--- /dev/null+++ b/src/test/isolation/expected/skip-locked-5.out@@ -0,0 +1,14 @@+Parsed test spec with 2 sessions++starting permutation: s1a s2a s1b s2b+step s1a: SELECT * FROM foo ORDER BY batch FETCH FIRST ROW WITH TIES FOR UPDATE SKIP LOCKED;+id batch ++1 1 +2 1 +step s2a: SELECT * FROM foo ORDER BY batch FETCH FIRST ROW WITH TIES FOR UPDATE SKIP LOCKED;+id batch ++3 2 +step s1b: COMMIT;+step s2b: COMMIT;diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_scheduleindex aa386ab1a2..9e32b00149 100644--- a/src/test/isolation/isolation_schedule+++ b/src/test/isolation/isolation_schedule@@ -64,6 +64,7 @@ test: skip-locked
test: skip-locked-2
test: skip-locked-3
test: skip-locked-4
+test: skip-locked-5
test: drop-index-concurrently-1
test: multiple-cic
test: alter-table-1
diff --git a/src/test/isolation/specs/skip-locked-5.spec b/src/test/isolation/specs/skip-locked-5.specnew file mode 100644index 0000000000..83bb3de00f--- /dev/null+++ b/src/test/isolation/specs/skip-locked-5.spec@@ -0,0 +1,29 @@+# Test SKIP LOCKED with FETCH FIRST ROW WITH TIES++setup+{+ CREATE TABLE foo (+ id int PRIMARY KEY,+ batch int NOT NULL+ );+ INSERT INTO foo VALUES (1, 1), (2, 1), (3, 2), (4, 3);+}++teardown+{+ DROP TABLE foo;+}++session "s1"+setup { BEGIN; }+step "s1a" { SELECT * FROM foo ORDER BY batch FETCH FIRST ROW WITH TIES FOR UPDATE SKIP LOCKED; }+step "s1b" { COMMIT; }++session "s2"+setup { BEGIN; }+step "s2a" { SELECT * FROM foo ORDER BY batch FETCH FIRST ROW WITH TIES FOR UPDATE SKIP LOCKED; }+step "s2b" { COMMIT; }++# s1 returns the two rows associated with batch 1 and takes the lock; s2 should skip those two rows due to being locked and should return the batch 2 row.+permutation "s1a" "s2a" "s1b" "s2b"+--
2.24.3 (Apple Git-128)
=
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: david@endpoint.com, tgl@sss.pgh.pa.us, alvherre@alvh.no-ip.org, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16676: SELECT ... FETCH FIRST ROW WITH TIES FOR UPDATE SKIP LOCKED locks rows it doesn't return
In-Reply-To: <C0CF9D56-0392-43D2-8577-5F8A01AF2B56@endpoint.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox