Skip to content

Commit f49c602

Browse files
craig[bot]michae2
craig[bot]
and
michae2
committed
Merge #152245
152245: optbuilder: take FOR SHARE locking during RC FK child checks r=rytaft a=michae2 **logictest: fix fk_cascade_race_150282** In #150291 we added subtest `fk_cascade_race_150282` which exercises FK checks performed by concurrent Serializable and Read Committed transactions. Unfortunately thanks to this test we've discovered that the locking added in #150291 is insufficient for preventing all FK violations. We need to turn on the disabled parent-FK-check locking for the Serializable transaction. This commmit fixes subtest `fk_cascade_race_150282` by turning on these settings for the Serializable transaction: - `enable_implicit_fk_locking_for_serializable` - `enable_shared_locking_for_serializable` - `enable_durable_locking_for_serializable` Informs: #151663 Release note: None --- **optbuilder: take FOR SHARE locking during RC FK child checks** Under Read Committed isolation, we need to take FOR SHARE locking during FK child checks to ensure that the rows read by the FK check have not already been written to at a later timestamp. (This is the same reason that we needed to add FOR UPDATE locking to RC FK cascades in #150291.) Even with this locking, we still need Serializable transactions to have the FK parent locking enabled, if both SSI and RC transactions are going to concurrently modify rows involved in FK relationships. This is because FOR SHARE (and FOR UPDATE) locking does not prevent phantoms. Informs: #151663 Release note (bug fix): A bug that would allow FK violations as a result of some combinations of concurrent Read Committed and Serializable transactions has been fixed. Note that if both Serializable and weaker-isolation transactions will concurrently modify rows involved in FK relationships, the Serializable transactions must have the following session variables set to prevent any possible FK violations: - `SET enable_implicit_fk_locking_for_serializable = on;` - `SET enable_shared_locking_for_serializable = on;` - `SET enable_durable_locking_for_serializable = on;` Co-authored-by: Michael Erickson <[email protected]>
2 parents 6228e69 + 3af3d3b commit f49c602

File tree

3 files changed

+229
-9
lines changed

3 files changed

+229
-9
lines changed

‎pkg/ccl/logictestccl/testdata/logic_test/fk_read_committed‎

Lines changed: 206 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,9 @@ WITH sleep AS (SELECT pg_sleep(1)) DELETE FROM parent_150282@parent_150282_i_idx
140140
user testuser
141141

142142
statement ok
143+
SET enable_implicit_fk_locking_for_serializable = on;
144+
SET enable_shared_locking_for_serializable = on;
145+
SET enable_durable_locking_for_serializable = on;
143146
INSERT INTO child_150282 VALUES (4, 1);
144147

145148
user root
@@ -176,6 +179,9 @@ WITH sleep AS (SELECT pg_sleep(1)) UPDATE parent_150282 SET p = 4 WHERE i = 2;
176179
user testuser
177180

178181
statement ok
182+
SET enable_implicit_fk_locking_for_serializable = on;
183+
SET enable_shared_locking_for_serializable = on;
184+
SET enable_durable_locking_for_serializable = on;
179185
INSERT INTO child_150282 VALUES (4, 1);
180186

181187
user root
@@ -219,6 +225,9 @@ WITH sleep AS (SELECT pg_sleep(1)) DELETE FROM parent_150282 WHERE p = 1;
219225
user testuser
220226

221227
statement ok
228+
SET enable_implicit_fk_locking_for_serializable = on;
229+
SET enable_shared_locking_for_serializable = on;
230+
SET enable_durable_locking_for_serializable = on;
222231
INSERT INTO child_150282 VALUES (4, 1);
223232

224233
user root
@@ -236,4 +245,201 @@ query II
236245
SELECT * FROM child_150282;
237246
----
238247

248+
user testuser
249+
250+
statement ok
251+
RESET enable_implicit_fk_locking_for_serializable;
252+
RESET enable_shared_locking_for_serializable;
253+
RESET enable_durable_locking_for_serializable
254+
255+
user root
256+
257+
subtest end
258+
259+
subtest fk_cascade_race_151663
260+
261+
statement ok
262+
CREATE TABLE parent_151663 (
263+
p INT PRIMARY KEY,
264+
q INT
265+
);
266+
267+
statement ok
268+
CREATE TABLE child_151663 (
269+
c INT PRIMARY KEY,
270+
p INT REFERENCES parent_151663 (p),
271+
INDEX (p),
272+
FAMILY (c, p)
273+
);
274+
275+
statement ok
276+
CREATE USER testuser2;
277+
CREATE USER testuser3
278+
279+
statement ok
280+
GRANT ALL ON TABLE parent_151663 TO testuser;
281+
GRANT ALL ON TABLE parent_151663 TO testuser2;
282+
GRANT ALL ON TABLE parent_151663 TO testuser3;
283+
GRANT ALL ON TABLE child_151663 TO testuser3
284+
285+
statement ok
286+
INSERT INTO parent_151663 VALUES (1, 0), (2, 0), (3, 0)
287+
288+
# The timeline of this test is:
289+
#
290+
# testuser testuser2 testuser3 root
291+
# -------- --------- --------- ----
292+
# begin
293+
# update p=2
294+
# begin
295+
# lock p=3
296+
# begin RC
297+
# start DELETE
298+
# wait on p=3
299+
# begin SSI
300+
# start INSERT
301+
# wait on p=2
302+
# rollback
303+
# lock p=1, p=2
304+
# rollback
305+
# wait on p=1
306+
# insert c=10, c=20
307+
# finish INSERT
308+
# commit
309+
# delete p=1
310+
# FK check child
311+
# finish DELETE
312+
# commit
313+
#
314+
# The transactions run by testuser3 and root are the important part. testuser
315+
# and testuser2 only exist to control the timing of the transactions run by
316+
# testuser3 and root.
317+
#
318+
# The transactions run by testuser3 and root conflict with each other. To
319+
# prevent a FK violation they need to either run serially, or one transaction
320+
# needs to fail.
321+
#
322+
# This test demonstrates that we need locking during both the parent FK check
323+
# and the child FK check to prevent a FK violation.
324+
#
325+
# The parent FK check is performed by the Serializable INSERT run by
326+
# root. Without locking during this check, the two transactions are not
327+
# guaranteed to detect the conflict with each other. For example, without this
328+
# locking, the Read Committed DELETE could finish after the INSERT's FK check
329+
# but before its insert. The INSERT could then write a phantom child row and
330+
# have a successful read refresh, and both transactions could commit. Locking
331+
# during the parent FK check avoids this by forcing the transactions to
332+
# coordinate on the parent row.
333+
#
334+
# The child FK check is performed by the Read Committed DELETE run by
335+
# testuser3. Without locking during this check, the Read Committed DELETE could
336+
# perform a stale read for its child FK check, missing a newer child row. For
337+
# example, it could miss the row written by the Serializable INSERT and
338+
# successfully commit. (Serializable isolation does not have this same risk of
339+
# stale reads and thus does not need locking during child FK checks.)
340+
#
341+
# For more discussion of this and other scenarios please see #151663.
342+
343+
user testuser
344+
345+
statement ok
346+
BEGIN
347+
348+
# Use an update on p=2 to block the future insert by root.
349+
statement ok
350+
UPDATE parent_151663 SET q = q + 1 WHERE p = 2
351+
352+
user testuser2
353+
354+
statement ok
355+
BEGIN
356+
357+
# Use a lock on p=3 to block the future delete by testuser3.
358+
statement ok
359+
SELECT * FROM parent_151663 WHERE p = 3 FOR UPDATE
360+
361+
user testuser3
362+
363+
statement ok
364+
BEGIN ISOLATION LEVEL READ COMMITTED
365+
366+
statement ok
367+
SELECT 1
368+
369+
statement async fkdelete error pgcode 23503 delete on table "parent_151663" violates foreign key constraint "child_151663_p_fkey" on table "child_151663"\nDETAIL: Key \(p\)=\(1\) is still referenced from table "child_151663"\.
370+
DELETE FROM parent_151663 WHERE p = (SELECT 1 FROM parent_151663 WHERE p = 3 FOR UPDATE)
371+
372+
user root
373+
374+
# Give the delete a moment to wait on the p=3 lock by testuser2.
375+
statement ok
376+
SELECT pg_sleep(1)
377+
378+
# The serializable insert needs this locking to properly sychronize with the
379+
# read committed delete.
380+
statement ok
381+
SET enable_implicit_fk_locking_for_serializable = on;
382+
SET enable_shared_locking_for_serializable = on;
383+
SET enable_durable_locking_for_serializable = on
384+
385+
statement ok
386+
BEGIN ISOLATION LEVEL SERIALIZABLE
387+
388+
statement async fkinsert
389+
INSERT INTO child_151663 VALUES (10, 1), (20, 2)
390+
391+
user testuser
392+
393+
# Give the insert a moment to wait on the p=2 update by testuser.
394+
statement ok
395+
SELECT pg_sleep(1)
396+
397+
statement ok
398+
ROLLBACK
399+
400+
user testuser2
401+
402+
# Give the insert a moment to lock p=1.
403+
statement ok
404+
SELECT pg_sleep(1)
405+
406+
statement ok
407+
ROLLBACK
408+
409+
user root
410+
411+
awaitstatement fkinsert
412+
413+
statement ok
414+
COMMIT
415+
416+
user testuser3
417+
418+
awaitstatement fkdelete
419+
420+
statement ok
421+
COMMIT
422+
423+
user root
424+
425+
# Check that there are no orphan children.
426+
427+
query II
428+
SELECT * FROM parent_151663 ORDER BY p
429+
----
430+
1 0
431+
2 0
432+
3 0
433+
434+
query II
435+
SELECT * FROM child_151663 ORDER BY c
436+
----
437+
10 1
438+
20 2
439+
440+
statement ok
441+
RESET enable_implicit_fk_locking_for_serializable;
442+
RESET enable_shared_locking_for_serializable;
443+
RESET enable_durable_locking_for_serializable
444+
239445
subtest end

‎pkg/sql/opt/exec/execbuilder/testdata/fk_read_committed‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,9 @@ vectorized: true
192192
estimated row count: 1 (missing stats)
193193
label: buffer 1
194194

195-
# Foreign key checks of the child do not require locking.
195+
# Foreign key checks of the child do not require locking under serializable
196+
# isolation, but do require locking under read committed isolation to avoid
197+
# stale reads.
196198
query T
197199
EXPLAIN (OPT) UPDATE jars SET j = j + 4
198200
----
@@ -211,7 +213,8 @@ update jars
211213
│ └── with-scan &1
212214
├── distinct-on
213215
│ └── scan cookies
214-
│ └── flags: avoid-full-scan
216+
│ ├── flags: avoid-full-scan
217+
│ └── locking: for-share,durability-guaranteed
215218
└── filters
216219
└── j = cookies.j
217220

@@ -362,6 +365,8 @@ vectorized: true
362365
estimated row count: 1,000 (missing stats)
363366
table: cookies@cookies_pkey
364367
spans: FULL SCAN
368+
locking strength: for share
369+
locking durability: guaranteed
365370

366371
query T
367372
EXPLAIN (OPT) DELETE FROM jars WHERE j = 1
@@ -375,7 +380,8 @@ delete jars
375380
└── semi-join (hash)
376381
├── with-scan &1
377382
├── scan cookies
378-
│ └── flags: avoid-full-scan
383+
│ ├── flags: avoid-full-scan
384+
│ └── locking: for-share,durability-guaranteed
379385
└── filters
380386
└── j = cookies.j
381387

@@ -444,6 +450,8 @@ vectorized: true
444450
│ estimated row count: 1,000 (missing stats)
445451
│ table: cookies@cookies_pkey
446452
│ spans: FULL SCAN
453+
│ locking strength: for share
454+
│ locking durability: guaranteed
447455
│
448456
└── • project
449457
│ columns: (j)
@@ -465,7 +473,8 @@ delete jars
465473
└── semi-join (hash)
466474
├── with-scan &1
467475
├── scan cookies
468-
│ └── flags: avoid-full-scan
476+
│ ├── flags: avoid-full-scan
477+
│ └── locking: for-share,durability-guaranteed
469478
└── filters
470479
└── j = cookies.j
471480

@@ -552,6 +561,8 @@ vectorized: true
552561
│ estimated row count: 1,000 (missing stats)
553562
│ table: cookies@cookies_pkey
554563
│ spans: FULL SCAN
564+
│ locking strength: for share
565+
│ locking durability: guaranteed
555566
│
556567
└── • project
557568
│ columns: (j)

‎pkg/sql/opt/optbuilder/mutation_builder_fk.go‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -640,11 +640,14 @@ func (h *fkCheckHelper) buildOtherTableScan(parent bool) (outScope *scope, tabMe
640640
// For insertion-side checks, if enable_implicit_fk_locking_for_serializable
641641
// is true or we're using a weaker isolation level, we lock the parent row(s)
642642
// to prevent concurrent mutations of the parent from other transactions from
643-
// violating the FK constraint. Deletion-side checks don't need to lock
644-
// because they can rely on the deletion intent conflicting with locks from
645-
// any concurrent inserts or updates of the child.
646-
if parent && (h.mb.b.evalCtx.TxnIsoLevel != isolation.Serializable ||
647-
h.mb.b.evalCtx.SessionData().ImplicitFKLockingForSerializable) {
643+
// violating the FK constraint. Deletion-side checks don't need to lock the
644+
// child row(s) under Serializable isolation because deletions can rely on the
645+
// deletion intent on the parent row conflicting with locks from any
646+
// concurrent inserts or updates of the child. Deletion-side checks do need to
647+
// lock the child row(s) under Read Committed isolation to ensure there are no
648+
// newer writers.
649+
if h.mb.b.evalCtx.TxnIsoLevel != isolation.Serializable ||
650+
(parent && h.mb.b.evalCtx.SessionData().ImplicitFKLockingForSerializable) {
648651
locking = lockingSpec{
649652
&lockingItem{
650653
item: &tree.LockingItem{

0 commit comments

Comments
 (0)