Merge #150291

150291: opt: take exclusive locks for foreign key cascades under read-committed r=mw5h a=mw5h

Previously foreign key cascades would never take locks on the rows they modified. Under serializable isolation, this presents no difficulty as locks are purely advisory at that isolation level. However, under repeatable read and read committed isolations, locks are necessary to avoid incorrect enforcement.

For example, consider this timing:

t1: R-C statement that deletes from parent starts. 
t2: transaction that writes to child commits. This row references the
    parent that the previous statement is deleting, but still sees it
    because that statement hasn't done anything yet.
t3: R-C statement deletes child keys, but has a timestamp of t1, so
    doesn't see the row inserted by t2.

After the R-C transaction commits, we're left with a child table that has a row it should not. A similar problem exists for update cascades.

Fortunately, the fix is relatively simple. By taking an exclusive lock on the child rows before we delete or update them, we force the KV to check write intents. KV will see the write at t2 and either bump t1's timestamp or force it to restart.

Fixes: #150282
Release note (bug fix): A but that would allow a race condition in foreign key cascades under read committed and repeatable read isolations has been fixed.

Co-authored-by: Matt White <[email protected]>

Author: craig[bot]

pkg/ccl/logictestccl/testdata/logic_test/fk_read_committed

@@ -98,3 +98,81 @@ INSERT INTO e VALUES (2)
 
 statement ok
 UPDATE a SET a = 2 WHERE a = 1
+
+subtest fk_cascade_race_150282
+
+statement ok
+CREATE TABLE parent_150282 (
+  p INT PRIMARY KEY,
+  i INT,
+  j INT,
+  INDEX (i),
+  INDEX (j)
+);
+
+statement ok
+CREATE TABLE child_150282 (
+  c INT PRIMARY KEY,
+  p INT REFERENCES parent_150282 (p) ON DELETE CASCADE ON UPDATE CASCADE,
+  INDEX (p)
+);
+
+statement ok
+INSERT INTO parent_150282 VALUES (1, 2, 3);
+
+statement ok
+BEGIN ISOLATION LEVEL READ COMMITTED;
+
+statement ok
+SELECT 1;
+
+statement async fk_delete
+WITH sleep AS (SELECT pg_sleep(1)) DELETE FROM parent_150282@parent_150282_i_idx WHERE i = 2;
+
+statement ok
+INSERT INTO child_150282 VALUES (4, 1);
+
+awaitstatement fk_delete
+
+statement ok
+COMMIT;
+
+query III
+SELECT * FROM parent_150282;
+----
+
+query II
+SELECT * FROM child_150282;
+----
+
+statement ok
+INSERT INTO parent_150282 VALUES (1, 2, 3);
+
+statement ok
+BEGIN ISOLATION LEVEL READ COMMITTED;
+
+statement ok
+SELECT 1;
+
+statement async fk_update
+WITH sleep AS (SELECT pg_sleep(1)) UPDATE parent_150282 SET p = 4 WHERE i = 2;
+
+statement ok
+INSERT INTO child_150282 VALUES (4, 1);
+
+awaitstatement fk_update
+
+statement ok
+COMMIT;
+
+query III
+SELECT * FROM parent_150282;
+----
+4 2 3
+
+query II
+SELECT * FROM child_150282;
+----
+4 4
+
+subtest end

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

@@ -6,6 +6,9 @@ CREATE TABLE jars (j INT PRIMARY KEY)
 statement ok
 CREATE TABLE cookies (c INT PRIMARY KEY, j INT REFERENCES jars (j), FAMILY (c, j))
 
+statement ok
+CREATE TABLE gumballs (g INT PRIMARY KEY, j INT REFERENCES jars (j) ON DELETE CASCADE ON UPDATE CASCADE, FAMILY (g, j))
+
 statement ok
 SET SESSION CHARACTERISTICS AS TRANSACTION ISOLATION LEVEL READ COMMITTED
 
@@ -243,6 +246,73 @@ vectorized: true
 │                 spans: FULL SCAN
 │                 locking strength: for update
 │
+├── • fk-cascade
+│   │ fk: gumballs_j_fkey
+│   │
+│   └── • root
+│       │ columns: ()
+│       │
+│       ├── • update
+│       │   │ columns: ()
+│       │   │ estimated row count: 0 (missing stats)
+│       │   │ table: gumballs
+│       │   │ set: j
+│       │   │
+│       │   └── • buffer
+│       │       │ columns: (g, j, j_new, j)
+│       │       │ label: buffer 1
+│       │       │
+│       │       └── • hash join (inner)
+│       │           │ columns: (g, j, j_new, j)
+│       │           │ estimated row count: 327 (missing stats)
+│       │           │ equality: (j) = (j)
+│       │           │
+│       │           ├── • scan
+│       │           │     columns: (g, j)
+│       │           │     estimated row count: 1,000 (missing stats)
+│       │           │     table: gumballs@gumballs_pkey
+│       │           │     spans: FULL SCAN
+│       │           │     locking strength: for update
+│       │           │     locking durability: guaranteed
+│       │           │
+│       │           └── • filter
+│       │               │ columns: (j, j_new)
+│       │               │ estimated row count: 33
+│       │               │ filter: j IS DISTINCT FROM j_new
+│       │               │
+│       │               └── • scan buffer
+│       │                     columns: (j, j_new)
+│       │                     estimated row count: 100
+│       │                     label: buffer 1000000
+│       │
+│       └── • constraint-check
+│           │
+│           └── • error if rows
+│               │ columns: ()
+│               │
+│               └── • lookup join (anti)
+│                   │ columns: (j_new)
+│                   │ estimated row count: 0 (missing stats)
+│                   │ table: jars@jars_pkey
+│                   │ equality: (j_new) = (j)
+│                   │ equality cols are key
+│                   │ locking strength: for share
+│                   │ locking durability: guaranteed
+│                   │ parallel
+│                   │
+│                   └── • filter
+│                       │ columns: (j_new)
+│                       │ estimated row count: 323 (missing stats)
+│                       │ filter: j_new IS NOT NULL
+│                       │
+│                       └── • project
+│                           │ columns: (j_new)
+│                           │
+│                           └── • scan buffer
+│                                 columns: (g, j, j_new, j)
+│                                 estimated row count: 327 (missing stats)
+│                                 label: buffer 1
+│
 └── • constraint-check
     │
     └── • error if rows
@@ -330,6 +400,30 @@ vectorized: true
 │             spans: /1/0
 │             locking strength: for update
 │
+├── • fk-cascade
+│   │ fk: gumballs_j_fkey
+│   │
+│   └── • delete
+│       │ columns: ()
+│       │ estimated row count: 0 (missing stats)
+│       │ from: gumballs
+│       │
+│       └── • project
+│           │ columns: (g)
+│           │
+│           └── • filter
+│               │ columns: (g, j)
+│               │ estimated row count: 10 (missing stats)
+│               │ filter: j = 1
+│               │
+│               └── • scan
+│                     columns: (g, j)
+│                     estimated row count: 1,000 (missing stats)
+│                     table: gumballs@gumballs_pkey
+│                     spans: FULL SCAN
+│                     locking strength: for update
+│                     locking durability: guaranteed
+│
 └── • constraint-check
     │
     └── • error if rows

pkg/sql/opt/optbuilder/fk_cascade.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/isolation"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
@@ -346,6 +347,18 @@ func (cb *onDeleteFastCascadeBuilder) Build(
 				indexFlags = &tree.IndexFlags{AvoidFullScan: true}
 			}
 
+			locking := noRowLocking
+			if b.evalCtx.TxnIsoLevel != isolation.Serializable {
+				locking = lockingSpec{
+					&lockingItem{
+						item: &tree.LockingItem{
+							Strength:   tree.ForUpdate,
+							WaitPolicy: tree.LockWaitBlock,
+						},
+					},
+				}
+			}
+
 			// Build the input to the delete mutation, which is simply a Scan with a
 			// Select on top. The scan is exempt from RLS to maintain data integrity.
 			mb.fetchScope = b.buildScan(
@@ -356,7 +369,7 @@ func (cb *onDeleteFastCascadeBuilder) Build(
 					includeInverted:  false,
 				}),
 				indexFlags,
-				noRowLocking,
+				locking,
 				b.allocScope(),
 				true, /* disableNotVisibleIndex */
 				cat.PolicyScopeExempt,
@@ -604,6 +617,18 @@ func (b *Builder) buildDeleteCascadeMutationInput(
 		indexFlags = &tree.IndexFlags{AvoidFullScan: true}
 	}
 
+	locking := noRowLocking
+	if b.evalCtx.TxnIsoLevel != isolation.Serializable {
+		locking = lockingSpec{
+			&lockingItem{
+				item: &tree.LockingItem{
+					Strength:   tree.ForUpdate,
+					WaitPolicy: tree.LockWaitBlock,
+				},
+			},
+		}
+	}
+
 	// The scan is exempt from RLS to maintain data integrity.
 	outScope = b.buildScan(
 		b.addTable(childTable, childTableAlias),
@@ -613,7 +638,7 @@ func (b *Builder) buildDeleteCascadeMutationInput(
 			includeInverted:  false,
 		}),
 		indexFlags,
-		noRowLocking,
+		locking,
 		b.allocScope(),
 		true, /* disableNotVisibleIndex */
 		cat.PolicyScopeExempt,
@@ -880,6 +905,18 @@ func (b *Builder) buildUpdateCascadeMutationInput(
 		indexFlags = &tree.IndexFlags{AvoidFullScan: true}
 	}
 
+	locking := noRowLocking
+	if b.evalCtx.TxnIsoLevel != isolation.Serializable {
+		locking = lockingSpec{
+			&lockingItem{
+				item: &tree.LockingItem{
+					Strength:   tree.ForUpdate,
+					WaitPolicy: tree.LockWaitBlock,
+				},
+			},
+		}
+	}
+
 	// The scan is exempt from RLS to maintain data integrity.
 	outScope = b.buildScan(
 		b.addTable(childTable, childTableAlias),
@@ -889,7 +926,7 @@ func (b *Builder) buildUpdateCascadeMutationInput(
 			includeInverted:  false,
 		}),
 		indexFlags,
-		noRowLocking,
+		locking,
 		b.allocScope(),
 		true, /* disableNotVisibleIndex */
 		cat.PolicyScopeExempt,