sql: fix index corruption in udfs with fk cascades

[rharding6373]

This PR fixes an issue in which UDFs making a FK cascade could cause index corruption. It adds inbound foreign key origin tables to the statement tree, which is used to test for tables with multiple mutations.

No release note is necessary, since UDF mutation and FK cascade support is new in this release.

Epic: CRDB-25388
Informs: #87289

Release note: None

[cockroach-teamcity]

This change is 

[DrewKimball]

Reviewed 4 of 5 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[michae2]

Thanks for working on this!

I think this is slightly too strict. IIUC, I think it will disallow the following, which, unlike this example, does not cause index corruption:

CREATE TABLE jar (j INT PRIMARY KEY);
CREATE TABLE cookie (i INT PRIMARY KEY, j INT REFERENCES jar (j) ON UPDATE CASCADE ON DELETE CASCADE, INDEX (j));
INSERT INTO jar VALUES (0), (2);
INSERT INTO cookie VALUES (0, 0);

CREATE FUNCTION f2(k INT) RETURNS INT AS $$
  UPDATE cookie SET j = 2 WHERE i = k RETURNING i
$$ LANGUAGE SQL;

-- this should not cause corruption, and should be allowed
-- (the cascade to cookie will always be strictly after the function call)
UPDATE jar SET j = j + 1 WHERE j = f2(0);

-- can check by comparing
SELECT i, j FROM cookie@primary;
SELECT i, j FROM cookie@cookie_j_idx;

Because we step the transaction sequence point before cascades, I think we need to do the equivalent thing and call statementTree.Pop before calling statementTree.CanMutateTable with any cascaded tables. But I'm not quite sure if that can be done safely in this part of optbuilder. 🤔

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[michae2]

One way to do it would be to walk the set of FK cascades immediately after the current call to Pop. (Or alternatively, maybe it could somehow become part of Pop if that would be simpler?)

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

[mgartner]

I think you're correct @michae2 (I may need some more time to convince myself, though 😕 ). Do you think this pattern is common enough in a real-world use case to worry about for now? I'm skeptical that using mutating UDFs in a filter is common or useful. If it is used, I wonder if the behavior is well-understood by the user since it's so hard to wrap your brain around. Maybe we can just slap a TODO and/or make an issue and call it a day for now?

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rharding6373)

---

pkg/sql/opt/optbuilder/util.go line 517 at r2 (raw file):

}

func (b *Builder) checkMultipleMutations(tab cat.Table, typ mutationType, visited intsets.Fast) {

nit: You could add a small non-recursive function that doesn't require a visited arg since all the callers are passing in an empty set.

---

pkg/sql/opt/optbuilder/util.go line 542 at r2 (raw file):

		fk := tab.InboundForeignKey(i)
		if (fk.DeleteReferenceAction() != tree.NoAction && typ != simpleInsert) ||
			fk.UpdateReferenceAction() != tree.NoAction {

I believe we can avoid tree.Restrict actions too, since they don't mutate tables. I believe we support RESTRICT cascades, but I haven't verified, but noticed this:

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

Lines 127 to 131 in 2675c7c

	// The action dictates how a foreign key reference is handled:
	// - with Cascade/SetNull/SetDefault, we create a cascading mutation to
	// modify or delete "orphaned" rows in the child table.
	// - with Restrict/NoAction, we create a check that causes an error if
	// there are any "orphaned" rows in the child table.

[rharding6373]

I did an equivalent to @michae2 's suggestion, which is to add a bool to CanMutateTable to indicate that this mutation is evaluated at the end of the statement. When true, it adds the mutation to the parent statement (if it exists) instead of the current statement. It appears to work. I need to add update the unit tests to exercise the new behavior, but wanted to get feedback/CI results due to the added complexity.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, and @michae2)

---

pkg/sql/opt/optbuilder/util.go line 517 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: You could add a small non-recursive function that doesn't require a visited arg since all the callers are passing in an empty set.

Refactored.

---

pkg/sql/opt/optbuilder/util.go line 542 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I believe we can avoid tree.Restrict actions too, since they don't mutate tables. I believe we support RESTRICT cascades, but I haven't verified, but noticed this:

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

Lines 127 to 131 in 2675c7c

	// The action dictates how a foreign key reference is handled:
	// - with Cascade/SetNull/SetDefault, we create a cascading mutation to
	// modify or delete "orphaned" rows in the child table.
	// - with Restrict/NoAction, we create a check that causes an error if
	// there are any "orphaned" rows in the child table.

Good call, done and added to the test case.

[mgartner]

Very nice!

Reviewed 1 of 2 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @rharding6373)

---

pkg/sql/opt/optbuilder/statement_tree.go line 152 at r3 (raw file):

	if isPostStmt {
		offset = 2
	}

nit: A short explanation of why we're checking two statements up the tree in the isPostStmt case might be helpful.

[michae2]

Nice! I think this just needs a small adjustment, mentioned below.

Also, not sure if related to that adjustment, but when playing with your branch I seem to get different results for EXPLAIN vs no EXPLAIN. Not sure why that is. For example, this fails for me with the error (just adding EXPLAIN to the example above):

CREATE TABLE jar (j INT PRIMARY KEY);
CREATE TABLE cookie (i INT PRIMARY KEY, j INT REFERENCES jar (j) ON UPDATE CASCADE ON DELETE CASCADE, INDEX (j));
INSERT INTO jar VALUES (0), (2);
INSERT INTO cookie VALUES (0, 0);

CREATE FUNCTION f2(k INT) RETURNS INT AS $$
  UPDATE cookie SET j = 2 WHERE i = k RETURNING i
$$ LANGUAGE SQL;

EXPLAIN UPDATE jar SET j = j + 1 WHERE j = f2(0);

But passes without the EXPLAIN.

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @mgartner and @rharding6373)

---

pkg/sql/opt/optbuilder/statement_tree.go line 156 at r4 (raw file):

		// check evaluations, so all updates in the current statement will be
		// visible.
		offset = 2

Very clever. I think it just needs some slight adjustment: if we are adding directly to the parent, I think we should be putting it into the parent's childrenGeneralMutationTables instead of the parent's generalMutationTables. This is because we don't want it to conflict with any additional checks for the current statement.

Also, I don't think we should be checking curr.childrenConflictWithMutation when curr is the parent, nor checking n.conflictsWithMutation when i = len(st.stmts-1).

[michae2]

Reviewed 2 of 7 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 2 stale) (waiting on @rharding6373)

[rharding6373]

TFTR!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)