opt: support INSERT ON CONFLICT DO NOTHING with partial unique constraints

[mgartner]

opt: move INSERT DO NOTHING arbiter code to mutation_builder_arbiter.go

Arbiter-related code in pkg/sql/opt/optbuilder/insert.go has grown
unruly due to the added complexity of partial indexes and unique
constraints. This commit moves some arbiter-related functions to a new
file to accommodate the growth. It also breaks some anonymous closures
into independent functions, for clarity.

Release note: None

opt: pass column ordinals directly to arbiter building functions

This commit makes the arguments of buildAntiJoinForDoNothingArbiter and
buildDistinctOnForDoNothingArbiter more intuitive. Columns ordinals
are now passed directly to these functions, rather than a column count
and ordinal-returning callback.

Release note: None

opt: create arbiterPredicateHelper for picking partial index arbiters

This commit adds a new helper struct that can determine if a partial
index can be used as an arbiter based on the arbiter predicate of an
INSERT ON CONFLICT statement. This will also be a useful utility to
determine if partial unique constraints can be used as arbiters.

Release note: None

opt: support INSERT ON CONFLICT DO NOTHING with partial unique constraints

To support INSERT ON CONFLICT DO NOTHING statements on tables with
partial UNIQUE WITHOUT INDEX constraints, partial constraints are now
selected as arbiters. These arbiters are used to filter out insert rows
that would conflict with existing rows in the table.

Informs #59195

There is no release note because these constraints are gated behind the
experimental_enable_unique_without_index_constraints session variable.

Release note: None

[cockroach-teamcity]

This change is 

[RaduBerinde]

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

---

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

	// predicates.
	var tableScope *scope
	initializeTableScopeOnce := func() {

[nit] these inline methods suggest this code could benefit from separating various routines in a helper

---

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

			var err error
			arbiterFilters, err = mb.b.buildPartialIndexPredicate(
				tabMeta, tableScope, arbiterPredicate, "arbiter predicate",

This is fragile, this function requires that initializeTableScopeOnce was called already. It would be more robust to obfuscate the variable (e.g. tableScopeLazy) and make the function tableScope() return the scope (and use that everywhere).

[rytaft]

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

---

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

		var partialIndexDistinctCol *scopeColumn
		if isPartial {
			alias := fmt.Sprintf("upsert_partial_index_distinct%d", idx)

[nit] I feel like it would be cleaner to just pass the type as a string (e.g., "index" or "constraint"), and construct the alias inside the function. Another option would be to pass the type as an enum with a String() method, but maybe that's overkill.

---

pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 1339 at r1 (raw file):

insert uniq_partial_constraint_and_index
 ├── columns: <none>
 ├── arbiter constraints: unique_a

It's using the constraint, not the index

---

pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 1419 at r1 (raw file):

insert uniq_constraint_and_partial_index
 ├── columns: <none>
 ├── arbiter indexes: secondary

It's using the index, not the constraint

[mgartner]

I did quite a bit of refactoring. If this totally screws up the review diff, let me know and I can open a new PR.

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

---

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

Previously, rytaft (Rebecca Taft) wrote…

[nit] I feel like it would be cleaner to just pass the type as a string (e.g., "index" or "constraint"), and construct the alias inside the function. Another option would be to pass the type as an enum with a String() method, but maybe that's overkill.

The function would still need idx to know the integer suffix. Are you suggesting passing just that?

---

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

Previously, RaduBerinde wrote…

[nit] these inline methods suggest this code could benefit from separating various routines in a helper

Great point. I just did a pretty big refactoring of a lot of this arbiter code to facilitate this. Let me know what you think.

---

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

Previously, RaduBerinde wrote…

This is fragile, this function requires that initializeTableScopeOnce was called already. It would be more robust to obfuscate the variable (e.g. tableScopeLazy) and make the function tableScope() return the scope (and use that everywhere).

I think I've made this less fragile with the refactoring I've done. Unfortunately there's still 1 point of fragility in that we rely on the tableScope's Scan being built to retrieve partial index predicate from the Scan table's metadata. Notice the h.tableScope() in the arbiterPredicateHelper.partialIndexPredicate function. I'm open to ways to make this better, if you can think of any.

This was an optimization to reduce rebuilding partial index predicates unnecessarily that was added here: 8b7cf44

---

pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 1339 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

It's using the constraint, not the index

This baffled me for a while, until I realized that we have to use the norm directive so that the predicates are fully normalized, otherwise pred.IsTrue(), which we use to check for pseudo-partial indexes and constraints, won't work. Fixed.

---

pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 1419 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

It's using the index, not the constraint

Done.

[rytaft]

Nice cleanup!

Reviewed 6 of 6 files at r2, 7 of 7 files at r3, 2 of 2 files at r4, 5 of 5 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)

---

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

Previously, mgartner (Marcus Gartner) wrote…

The function would still need idx to know the integer suffix. Are you suggesting passing just that?

I see, sure, you could pass that too. But I don't feel too strongly -- if you don't feel like it's cleaner feel free to ignore. (you could also theoretically make a different alias, right? with the index/constraint name or something?)

---

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

			partialIndexDistinctCol,
		)

[nit] extra line

---

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

	uniqueCheckHelper uniqueCheckHelper

	// arbiterHelper is used to prevent allocating the helper separately.

arbiterHelper -> arbiterPredicateHelper

---

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

}

// buildInputForDoNothingArbiter builds the input for a single arbiter index or

name doesn't match function name. I think this whole comment may be mismatched....

---

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

	tableScopeLazy *scope

	// arbiterFilters is a lazily initialized scalar expression that represents

arbiterFilters -> arbiterFiltersLazy

[nit] what does "represents" mean in this context? maybe "is the result of building arbiterPredicate"?

---

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

			h.mb.b.allocScope(),
		)

[nit] extra line

---

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

// catalog, it fetches the predicates from the table metadata. These predicates
// are populated when the Scan expression is built for the tableScope. This
// eliminate unnecessarily rebuilding partial index predicate expressions.

eliminates

---

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

	// The filters have been initialized if they are non-nil or
	// invalidArbiterPredicate has been set to true.
	arbiterFiltersInitialized := h.arbiterFiltersLazy != nil || h.invalidArbiterPredicate

if we already know that the arbiter predicate is invalid, why are we trying to build it again? won't we just get the same result?

---

pkg/sql/opt/optbuilder/mutation_builder_arbiter.go, line 195 at r5 (raw file):

//     guarantees uniqueness of.
// 	 - pred is the partial index predicate. If the arbiter is not a partial
//     index, pred is nil.

[nit] index or constraint

---

pkg/sql/opt/optbuilder/testdata/unique-checks-insert, line 1339 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

This baffled me for a while, until I realized that we have to use the norm directive so that the predicates are fully normalized, otherwise pred.IsTrue(), which we use to check for pseudo-partial indexes and constraints, won't work. Fixed.

good find!

[mgartner]

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

---

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

Previously, rytaft (Rebecca Taft) wrote…

I see, sure, you could pass that too. But I don't feel too strongly -- if you don't feel like it's cleaner feel free to ignore. (you could also theoretically make a different alias, right? with the index/constraint name or something?)

I like that idea. I added a new commit that uses the index or constraint name instead.

---

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

Previously, rytaft (Rebecca Taft) wrote…

[nit] extra line

Done.

---

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

Previously, rytaft (Rebecca Taft) wrote…

arbiterHelper -> arbiterPredicateHelper

Done.

---

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

Previously, rytaft (Rebecca Taft) wrote…

name doesn't match function name. I think this whole comment may be mismatched....

Good catch. Must have mucked this up during some rebasing. Fixed.

---

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

Previously, rytaft (Rebecca Taft) wrote…

arbiterFilters -> arbiterFiltersLazy

[nit] what does "represents" mean in this context? maybe "is the result of building arbiterPredicate"?

Done.

---

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

Previously, rytaft (Rebecca Taft) wrote…

[nit] extra line

Done.

---

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

Previously, rytaft (Rebecca Taft) wrote…

eliminates

Done.

---

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

Previously, rytaft (Rebecca Taft) wrote…

if we already know that the arbiter predicate is invalid, why are we trying to build it again? won't we just get the same result?

If it's invalid then we won't try to build again because arbiterFiltersInitialized will be true, and we check if !arbiterFiltersInitialized below. This is confusing tho, maybe due to some double or triple negatives. This version was the least evil that I could come up with though.

---

pkg/sql/opt/optbuilder/mutation_builder_arbiter.go, line 195 at r5 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

[nit] index or constraint

Done.

[rytaft]

Reviewed 10 of 10 files at r6, 7 of 7 files at r7, 2 of 2 files at r8, 5 of 5 files at r9, 5 of 5 files at r10.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @RaduBerinde)

---

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

		var partialIndexDistinctCol *scopeColumn
		if isPartial {
			alias := fmt.Sprintf("upsert_partial_index_distinct%d", idx)

[nit] change this one to pass the index name too

---

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

Previously, mgartner (Marcus Gartner) wrote…

If it's invalid then we won't try to build again because arbiterFiltersInitialized will be true, and we check if !arbiterFiltersInitialized below. This is confusing tho, maybe due to some double or triple negatives. This version was the least evil that I could come up with though.

Oh I see -- yep, sorry I confused myself. I think this is fine.

[mgartner]

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

---

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

Previously, rytaft (Rebecca Taft) wrote…

[nit] change this one to pass the index name too

Done.

[rytaft]

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

[RaduBerinde]

Very nice cleanup!

Reviewed 1 of 10 files at r6, 3 of 7 files at r7, 1 of 2 files at r8, 1 of 5 files at r9, 5 of 5 files at r13.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @RaduBerinde)

---

pkg/sql/opt/optbuilder/mutation_builder_arbiter.go, line 429 at r8 (raw file):

	// Initialize the Implicator once.
	if h.im == nil {
		h.im = &partialidx.Implicator{}

[nit] Any reason im has to be a pointer? (btw it's ok to call Init every time since it doesn't do much)

[mgartner]

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

---

pkg/sql/opt/optbuilder/mutation_builder_arbiter.go, line 429 at r8 (raw file):

Previously, RaduBerinde wrote…

[nit] Any reason im has to be a pointer? (btw it's ok to call Init every time since it doesn't do much)

Nope, good catch. Fixed.

[mgartner]

TFTRs!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)