opt: fix FK cascades to child tables with partial indexes

[mgartner]

Previously, the optimizer was not synthesizing partial index DEL columns
for FK cascading updates and deletes. As a result, a cascading UPDATE
could corrupt a child table's partial index, ultimately resulting in
incorrect query results. A cascading DELETE would not corrupt
partial indexes, but unnecessary DEL operations would be issued on
the partial index.

The optbuilder has been refactored so that these columns are correctly
projected. There are now three functions for projecting PUT columns, DEL
columns, and both PUT and DEL columns, each ensuring that the input
scopes are non-nil. These three functions are called from principal
functions in the optbuilder where CHECK constraint columns are also
projected, like mutationBuilder.buildUpdate. In theory this should
make it harder in the future to omit these necessary projections.

Additionally, the execution engine was unable to handle extraneous
columns that can be added as input to FK cascading updates. These
extraneous columns would be incorrectly interpreted as synthesized
partial index columns. This commit works around this issue by slicing
the source values with an upper bound in updateNode.processSourceRow.
The longer term fix is to not produce these columns (see issue #57097).

Fixes #57085
Fixes #57084

Release justification: This is a critical bug fix to a new feature,
partial indexes.

Release note (bug fix): A bug has been fixed that caused errors or
corrupted partial indexes of child tables in foreign key relationships
with cascading UPDATEs and DELETEs. The corrupt partial indexes
could result in incorrect query results. Any partial indexes on child
tables of foreign key relationships with ON DELETE CASCADE or ON UPDATE CASCADE actions may be corrupt and should be dropped and
re-created. This bug was introduce in version 20.2.

[cockroach-teamcity]

This change is 

[mgartner]

Ooops. Forgot to include some optbuilder tests. I'll add those.

[mgartner]

There might also be problems with an ON CASCADE DELETE but I'm having trouble producing a test case exhibiting a bug.

[rytaft]

[nit] in the commit/PR message, this sentence is garbled: "... columns have been synthesized for from the optimizer to the execution engine ..."

There might also be problems with an ON CASCADE DELETE but I'm having trouble producing a test case exhibiting a bug.

Maybe you can use SHOW TRACE?

Reviewed 14 of 14 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)

---

pkg/sql/update.go, line 59 at r1 (raw file):

	checkOrds        checkSet
	partialIndexOrds partialIndexSet

Is this getting used other than for its length?

---

pkg/sql/logictest/testdata/logic_test/partial_index, line 1530 at r1 (raw file):

INSERT INTO t57085c_groups VALUES (1), (2);
INSERT INTO t57085c_users VALUES (10, 1, 100), (20, 2, 200);
UPDATE t57085c_groups SET id = 3 WHERE id = 1;

should there be a similar test to ensure that rows are deleted from the partial index if group_id is updated from 3->4?

---

pkg/sql/opt/exec/factory.opt, line 416 at r1 (raw file):

# transaction (if appropriate, i.e. if it is in an implicit transaction).
# This is false if there are multiple mutations in a statement, or the output
# of the mutation is processed through side-effecting expressions.

[nit] update comment to mention partial indexes

---

pkg/sql/opt/exec/execbuilder/mutation.go, line 343 at r1 (raw file):

		returnColOrds,
		checkOrds,
		partialIndexPutOrds,

why do we only need Put here and not Del?

---

pkg/sql/opt/optbuilder/fk_cascade.go, line 637 at r1 (raw file):

		)

		// Set the fetchScope to the scope of the fetch expression.

what's a fetch expression?

---

pkg/sql/opt/optbuilder/insert.go, line 908 at r1 (raw file):

	//       reason other than as "fetch columns". See buildScan comment.
	// TODO(andyk): Why does execution engine need mutation columns for Insert?
	fetchScope := mb.b.buildScan(

why not just use mb.fetchScope directly here instead of creating a new variable?

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 341 at r1 (raw file):

	}

	// Set the fetchScope to the scope of the fetch expression.

[nit] what's a fetch expression?

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 408 at r1 (raw file):

	mb.setFetchColIDs(mb.outScope.cols)

	// Set the fetchScope to the scope of the fetch expression.

ditto

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 822 at r1 (raw file):

			expr := mb.parsePartialIndexPredicateExpr(i)

			// Build a synthesized PUT columns.

[nit] a synthesized -> synthesized

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 832 at r1 (raw file):

			}

			// Build a synthesized DEL columns.

ditto

[mgartner]

Thanks for the feedback @rytaft. I reworked this quite a bit, so some of them were no longer applicable. I think this is more polished now.

The first commit is from #57153.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)

---

pkg/sql/update.go, line 59 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Is this getting used other than for its length?

No. I realized this wasn't necessary and I've removed it.

---

pkg/sql/logictest/testdata/logic_test/partial_index, line 1530 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

should there be a similar test to ensure that rows are deleted from the partial index if group_id is updated from 3->4?

Done.

---

pkg/sql/opt/exec/factory.opt, line 416 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] update comment to mention partial indexes

I've removed this new argument — it wasn't necessary.

---

pkg/sql/opt/exec/execbuilder/mutation.go, line 343 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why do we only need Put here and not Del?

This has been removed — it wasn't necessary.

---

pkg/sql/opt/optbuilder/fk_cascade.go, line 637 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

what's a fetch expression?

I've updated the comment for the fetchScope field, and redone a lot of this code. LMK if it's still unclear what fetchScope is.

---

pkg/sql/opt/optbuilder/insert.go, line 908 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

why not just use mb.fetchScope directly here instead of creating a new variable?

Done.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 341 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] what's a fetch expression?

See previous comment.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 408 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

ditto

See previous comment.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 822 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] a synthesized -> synthesized

Done.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 832 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

ditto

Done.

[rytaft]

Reviewed 6 of 15 files at r2, 18 of 18 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 797 at r3 (raw file):

// partialIndexProjectColumns specifies which types of partial index columns to
// project, PUT columns, DEL columns, or both. Different mutations require

[nit] project, -> project:

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 798 at r3 (raw file):

// partialIndexProjectColumns specifies which types of partial index columns to
// project, PUT columns, DEL columns, or both. Different mutations require
// different sets of these columns to be project. For example, INSERT does not

[nit] to be project -> to be projected

[RaduBerinde]

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

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 806 at r3 (raw file):

	putCols partialIndexProjectColumns = 1 << iota
	delCols
	putAndDelCols = putCols | delCols

[nit] putAndDelCols partialIndexProjectColumns = ..

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 811 at r3 (raw file):

// HasColumns tests whether pc contains all the partial index project columns in
// other.
func (pc partialIndexProjectColumns) HasColumns(other partialIndexProjectColumns) bool {

[nit] maybe just Has is a better name

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 817 at r3 (raw file):

// projectPartialIndexCols builds a Project that synthesizes boolean output
// columns for each partial index defined on the target table. The projectCols
// argument specifies if PUT columns, DEL columns, or both PUT and DEL columns

[nit] could use a description of what the scopes are

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 820 at r3 (raw file):

// should be projected.
func (mb *mutationBuilder) projectPartialIndexCols(
	putScope *scope, delScope *scope, projectCols partialIndexProjectColumns,

An alternative here would be to provide three variants (projectPartialIndex[Put|Del|PutAndDel]Cols) and rename this to projectPartialIndexColsImpl. The variants would check for nil scopes, and Impl could just check them against nil instead of using partialIndexProjectColumns.

[mgartner]

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @RaduBerinde and @rytaft)

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 797 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] project, -> project:

Remove this type entirely.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 798 at r3 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] to be project -> to be projected

Removed.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 806 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] putAndDelCols partialIndexProjectColumns = ..

Removed this const entirely.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 811 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] maybe just Has is a better name

Removed this function entirely.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 817 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] could use a description of what the scopes are

Done.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 820 at r3 (raw file):

Previously, RaduBerinde wrote…

An alternative here would be to provide three variants (projectPartialIndex[Put|Del|PutAndDel]Cols) and rename this to projectPartialIndexColsImpl. The variants would check for nil scopes, and Impl could just check them against nil instead of using partialIndexProjectColumns.

I love it. Done.

[RaduBerinde]

Reviewed 13 of 18 files at r3, 5 of 5 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner, @RaduBerinde, and @rytaft)

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 811 at r4 (raw file):

// projectPartialIndexDelCols builds a Project that synthesizes boolean PUT
// columns for each partial index defined on the target table. See
// partialIndexPutColIDs for more info on these columns.

[nit] DelColIDs

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 837 at r4 (raw file):

	}
	if delScope == nil {
		panic(errors.AssertionFailedf("cannot project partial index PUT columns with nil scope"))

DEL

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @RaduBerinde and @rytaft)

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 811 at r4 (raw file):

Previously, RaduBerinde wrote…

[nit] DelColIDs

Done.

---

pkg/sql/opt/optbuilder/mutation_builder.go, line 837 at r4 (raw file):

Previously, RaduBerinde wrote…

DEL

Done. Fixed the one above in projectPartialIndexDelCols too.

[mgartner]

TFTRs!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)