sql: handle deletes for partial indexes by mgartner · Pull Request #50514 · cockroachdb/cockroach

mgartner · 2020-06-23T02:16:01Z

This commit makes DELETEs only attempt to remove entries from partial
indexes when the row exists in the partial index. For DELETEs, the
optimizer now synthesizes a boolean column for each partial index on the
table. This column evaluates to true if the partial index predicate
evaluates to true for the row being deleted, signifying that it exists
within the partial index and should be deleted.

There are now two sets of columns synthesized for partial index
mutations, PUT columns and DEL columns. The PUT columns communicate when
rows should be added to partial indexes. These are currently used by
INSERT and in the future will be used by UPDATE and UPSERT. The
DEL columns communicate when rows shold be removed from partial indexes.
These are currently used by DELETE, and in the future will be used by
UPDATE and UPSERT.

Release note: None

cockroach-teamcity · 2020-06-23T02:16:08Z

This change is

RaduBerinde

Very nice, this was simpler than I expected.

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

pkg/sql/delete.go, line 47 at r1 (raw file):

	// fetchCols are the columns being fetched.
	fetchCols []sqlbase.ColumnDescriptor

We are only using the length of this no? Would be nicer to just make it more specific, e.g. partialIndexDelValsOffset)

pkg/sql/delete.go, line 184 at r1 (raw file):

	indexes := d.run.td.tableDesc().Indexes
	for i := range indexes {
		if colIdx >= len(partialIndexDelVals) {

[nit] feels like this belongs right after colIdx++

pkg/sql/delete.go, line 198 at r1 (raw file):

				// is false, the row should not be removed from the partial
				// index.
				ignoreIndexes.Add(int(index.ID))

I guess it is now critical that the predicate is a fully immutable expression.. If we wanted for whatever crazy reason to support stable expressions, we would have to always try to delete the keys.

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

// index defined on the target table. Each synthesized column is prefixed with
// aliasPrefix and added to the ords list.
func (mb *mutationBuilder) addPartialIndexCols(aliasPrefix string, ords *[]scopeOrdinal) {

why is ords a pointer? we're only modifying the elements, not the slice itself

mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/delete.go, line 47 at r1 (raw file):

Previously, RaduBerinde wrote…

We are only using the length of this no? Would be nicer to just make it more specific, e.g. partialIndexDelValsOffset)

Done.

pkg/sql/delete.go, line 184 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] feels like this belongs right after colIdx++

Done.

pkg/sql/delete.go, line 198 at r1 (raw file):

Previously, RaduBerinde wrote…

I guess it is now critical that the predicate is a fully immutable expression.. If we wanted for whatever crazy reason to support stable expressions, we would have to always try to delete the keys.

Correct. I don't think it's really possible to support stable functions. Imagine CREATE INDEX i ON t (a) WHERE foo(). If foo() is stable, then the contents of the partial index are not fully defined. And if the contents are not defined, it can't be scanned to satisfy a query and produce any sensible results.

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

Previously, RaduBerinde wrote…

why is ords a pointer? we're only modifying the elements, not the slice itself

good point, my mistake.

mgartner · 2020-06-25T17:01:17Z

pkg/sql/opt/optbuilder/delete.go

 func (mb *mutationBuilder) buildDelete(returning tree.ReturningExprs) {
+	// Disambiguate names so that references in any expressions, such as a
+	// partial index predicate, refer to the correct columns.
+	mb.disambiguateColumns()


Is this necessary in this case? I think so, but I'm not fully certain about the purpose of disambiguateColumns.

mgartner · 2020-06-25T17:02:24Z

pkg/sql/opt/norm/testdata/rules/prune_cols

-      │         └── projections
-      │              └── i:6 * 2 [as=column9:9, outer=(6), immutable]
-      └── 10
+      └── limit


I'm not entirely sure why this project was added (or is no longer removed).

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @andy-kimball and @mgartner)

pkg/sql/opt/norm/testdata/rules/prune_cols, line 1856 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I'm not entirely sure why this project was added (or is no longer removed).

I would use optsteps to figure it out. (and maybe check if there's any difference in build output). The Limit shouldn't be advertising column9 as pruneable, so not sure how the project gets there.

pkg/sql/opt/optbuilder/delete.go, line 79 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is this necessary in this case? I think so, but I'm not fully certain about the purpose of disambiguateColumns.

Not sure, @andy-kimball should know

pkg/sql/opt/optbuilder/delete.go, line 83 at r2 (raw file):

	mb.buildFKChecksAndCascadesForDelete()

	// Add partial index boolean columns to the input.

I think this should logically come before buildFKChecks

RaduBerinde · 2020-06-25T17:22:49Z

pkg/sql/opt/norm/prune_cols_funcs.go, line 63 at r2 (raw file):

	addCols(private.UpdateCols)
	addCols(private.CheckCols)
	addCols(private.PartialIndexPutCols)

Do we have a test where we are deleting from a table and we wouldn't have fetched a column without a partial index, but we have to only because it's used by the partial index predicate?

mgartner

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

pkg/sql/opt/norm/prune_cols_funcs.go, line 63 at r2 (raw file):

Previously, RaduBerinde wrote…

Do we have a test where we are deleting from a table and we wouldn't have fetched a column without a partial index, but we have to only because it's used by the partial index predicate?

Nope, good call. I added a test to norm/testdata/rules/prune_cols for this.

pkg/sql/opt/norm/testdata/rules/prune_cols, line 1856 at r2 (raw file):

Previously, RaduBerinde wrote…

I would use optsteps to figure it out. (and maybe check if there's any difference in build output). The Limit shouldn't be advertising column9 as pruneable, so not sure how the project gets there.

Turns out this was because addPartialIndexCols was always adding a Project, even if there were no partial indexes on the table. EliminateProject was not firing on this new Project because the passthrough columns (5) were not equal to the limit columns (5, 9).

I've updated it so that a projection is only added if there is one or more partial indexes defined on the table.

pkg/sql/opt/optbuilder/delete.go, line 79 at r2 (raw file):

Previously, RaduBerinde wrote…

Not sure, @andy-kimball should know

We talked, and it is necessary to disambiguate columns, for example, when a column that is in a RETURNING clause is also in a partial index predicate expression.

pkg/sql/opt/optbuilder/delete.go, line 83 at r2 (raw file):

Previously, RaduBerinde wrote…

I think this should logically come before buildFKChecks

Done.

This commit makes `DELETE`s only attempt to remove entries from partial indexes when the row exists in the partial index. For `DELETE`s, the optimizer now synthesizes a boolean column for each partial index on the table. This column evaluates to true if the partial index predicate evaluates to true for the row being deleted, signifying that it exists within the partial index and should be deleted. There are now two sets of columns synthesized for partial index mutations, PUT columns and DEL columns. The PUT columns communicate when rows should be added to partial indexes. These are currently used by `INSERT` and in the future will be used by `UPDATE` and `UPSERT`. The DEL columns communicate when rows shold be removed from partial indexes. These are currently used by `DELETE`, and in the future will be used by `UPDATE` and `UPSERT`. Release note: None

RaduBerinde

Reviewable status: complete! 1 of 0 LGTMs obtained

mgartner · 2020-07-01T18:10:56Z

bors r= RaduBerinde

craig · 2020-07-01T19:10:36Z

Build succeeded

GitHub CI (Cockroach)

mgartner requested a review from a team as a code owner June 23, 2020 02:16

mgartner requested a review from RaduBerinde June 23, 2020 02:16

RaduBerinde approved these changes Jun 23, 2020

View reviewed changes

mgartner force-pushed the delete-partial-index branch from 35fa16d to e8f26fd Compare June 23, 2020 18:55

mgartner commented Jun 23, 2020

View reviewed changes

mgartner force-pushed the delete-partial-index branch 2 times, most recently from cfe8658 to 104228b Compare June 25, 2020 16:55

mgartner commented Jun 25, 2020

View reviewed changes

RaduBerinde reviewed Jun 25, 2020

View reviewed changes

mgartner force-pushed the delete-partial-index branch from 104228b to bbd8c03 Compare June 29, 2020 19:17

mgartner commented Jun 29, 2020

View reviewed changes

mgartner force-pushed the delete-partial-index branch from bbd8c03 to 9001afe Compare June 29, 2020 20:26

RaduBerinde reviewed Jul 1, 2020

View reviewed changes

craig bot merged commit c016727 into cockroachdb:master Jul 1, 2020

mgartner deleted the delete-partial-index branch July 1, 2020 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: handle deletes for partial indexes#50514

sql: handle deletes for partial indexes#50514
craig[bot] merged 1 commit intocockroachdb:masterfrom
mgartner:delete-partial-index

mgartner commented Jun 23, 2020

Uh oh!

cockroach-teamcity commented Jun 23, 2020

Uh oh!

RaduBerinde left a comment

Uh oh!

mgartner left a comment

Uh oh!

mgartner Jun 25, 2020

Uh oh!

mgartner Jun 25, 2020

Uh oh!

RaduBerinde left a comment

Uh oh!

RaduBerinde commented Jun 25, 2020

Uh oh!

mgartner left a comment

Uh oh!

RaduBerinde left a comment

Uh oh!

mgartner commented Jul 1, 2020

Uh oh!

craig bot commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mgartner commented Jun 23, 2020

Uh oh!

cockroach-teamcity commented Jun 23, 2020

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

mgartner Jun 25, 2020

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde commented Jun 25, 2020

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

RaduBerinde left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner commented Jul 1, 2020

Uh oh!

craig bot commented Jul 1, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants