Ya, this stuff is a bit wonky because we don't currently distinguish between a unique index and a unique constraint internally like Postgres does. As @ajwerner points out, that's a non-trivial thing to fix, and who knows when we will have the time to address it.

However, I don't think that blocks this PR. Postgres, like CRDB, does not allow a unique constraint and an index to share the same name. So there will never be ambiguity in an ON CONFLICT ON CONSTRAINT statement. Once we get around to fixing our handling of unique index and unique constraints, we can prevent users from choosing unique indexes as arbiter constraints in this new syntax.

are we accidentally reporting partial unique indexes as "constraints" when they shouldn't be, for example?

I assume you're referring to the contents of pg_catalog.pg_constraint? IIRC I did this to match the behavior of our unique indexes which appear in pg_constraint, even though this is inconsistent with Postgres. I think we could remove partial unique indexes from pg_constraint without causing issues, without waiting for #65825 to land. I'd defer to @rafiss on whether that's a good idea or not.

Another difference - when we create a partial unique index in Cockroach, it's given the _key suffix. In Postgres, it's given the _idx suffix, and cannot later be converted into a true "constraint". I'll write some of this stuff up later if you think it's useful, just wanted to jot down the findings for now here, though.

This should be a simple fix. I've created an issue to track: #73556

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

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

// and system columns are ignored.
func (mb *mutationBuilder) getArbitersFromOnConflict(onConflict *tree.OnConflict) arbiterSet {
	if onConflict == nil {

It would be cleaner to have a single entry point for getting arbiters (ideally mb.findArbiters) and handle the three separate logic paths within that function (breaking into helper functions if necessary). Most of the arbiter related code lives in mutation_builder_arbiter.go.

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

					panic(
						errors.WithHint(pgerror.Newf(pgcode.WrongObjectType, "unique constraint %q for table %q is partial, "+
							"so cannot be used as an arbiter index directly",

This error message is confusing because it can be used as an arbiter index, just not with the ON CONSTRAINT syntax.

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

# On conflict clause references unique without index constraint explicitly.
build
INSERT INTO uniq VALUES (1, 2, 3, 4, 5) ON CONFLICT ON CONSTRAINT unique_w DO NOTHING

nit: add a similar test in unique-checks-upsert with ... DO UPDATE SET ...

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

# Error when trying to select a partial unique without index constraint
# explicitly, which is not allowed.

nit: add a similar test in unique-checks-upsert with ... DO UPDATE SET ...

pkg/sql/opt/optbuilder/testdata/upsert, line 2313 at r2 (raw file):

VALUES (1, 2)
ON CONFLICT ON CONSTRAINT xyz_pkey DO
UPDATE SET x=5

nit: I'd sprinkle these tests in the Test UPDATE SET expressions. and Test DO NOTHING sections, right under a test with the same behavior, but the column-selection syntax.

Thanks for the commentary here!

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

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

Done. I renamed this function findArbiters, moved it to the file you named, and renamed the old findArbiters to be called inferArbitersFromConflictOrds.

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

Done.

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

I'm not sure what you mean here - I don't see a unique-checks-upsert test file, and I don't think upsert lets you do DO UPDATE SET... right? Sorry, I must be missing something.

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

Ditto above comment

pkg/sql/opt/optbuilder/testdata/upsert, line 2313 at r2 (raw file):

I looked at doing this, but in the end I think it's cleaner to have it in a separate section. So I won't take this nit unless you insist! :)

I didn't think it's necessary to separately test DO NOTHING because the code that handles the constraint selection is orthogonal to the code that handles the ACTION differences.

And the variety of testing I wanted to do here was different from the variety that we needed in the DO UPDATE SET cases (subquery sets and DEFAULT sets).

Very nice! Apologies for my slow response.

Reviewed 7 of 8 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)

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

// panics if no arbiters are found.
//
// Arbiter constraints ensure that the columns designated by conflictOrds

Can you reword this to include ON CONSTRAINT here an in the numbered bullets below? We should probably remove references to conflictOrds with something more general like "conflict columns" now that conflictOrds is no longer an argument.

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

				}
				arbiters.AddIndex(i)
				return arbiters

nit: you could simplify this to return makeSingleIndexArbiterSet(mb, i)

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

				}
				arbiters.AddUniqueConstraint(i)
				return arbiters

nit: you could simplify this to return makeSingleUniqueConstraintArbiterSet(mb, i)

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

		onConflict.Constraint, tableName),
		"use the ON CONFLICT (columns...) WHERE <predicate> form to select this partial unique constraint as an arbiter",
	)

nit: the linebreaks and indents of this looks off to me. might be more readable as:

return errors.WithHint(
    pgerror.Newf(
        pgcode.WrongObjectType,
        "unique constraint %q for table %q is partial, so cannot be used as an arbiter via the ON CONSTRAINT syntax",
        onConflict.Constraint,
        tableName,
    ),
    "use the ON CONFLICT (columns...) WHERE <predicate> form to select this partial unique constraint as an arbiter",
)

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

It's confusing. There's a few optimizer test files with "upsert" in the name that include INSERT ON CONFLICT DO UPDATE tests. For example:

You could add a test to that file similar to this test but instead of DO NOTHING, it uses DO UPDATE SET .... I guess you might not want to based on your other comment about arbiter selection being orthogonal to the conflict action. I'll leave it up to you to decide.

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

See my response.

pkg/sql/opt/optbuilder/testdata/upsert, line 2313 at r2 (raw file):

SGTM!