schemachanger: Implement `ADD UNIQUE WITHOUT INDEX` #93824

Xiang-Gu · 2022-12-16T22:07:53Z

This PR implements ADD UNIQUE WITHOUT INDEX in the declarative schema changer.
In general, it follows the same recipe for ADD CHECK.

Epic: None

cockroach-teamcity · 2022-12-16T22:08:13Z

This change is

Xiang-Gu · 2022-12-19T20:56:50Z

This is ready for a round of looks!

postamar · 2022-12-20T21:34:42Z

This looks good, I did a first pass and have no comments besides what I already shared in the PR this is based on, which might apply here also.

rytaft

Nice! Most of this code is out of my area of expertise, but from what I can tell it looks good!

With this PR have you added support for all aspects of UNIQUE WITHOUT INDEX or just some? e.g., does this cover adding NOT VALID constraints and/or running VALIDATE on an existing constraint? How about adding them as part of a CREATE TABLE statement or dropping them?

Also seems like the change to include indexIDForValidation is mixed in with this change but should probably be its own separate change. It doesn't seem related to this.

I left a bunch of nits about long comment lines. Comments should be <= 80 characters wide.

Reviewed 5 of 5 files at r18, 9 of 9 files at r19, 3 of 3 files at r20, 3 of 3 files at r21, 17 of 17 files at r22, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)

pkg/sql/catalog/rewrite/rewrite.go line 176 at r21 (raw file):

		}

		// Rewrite unique_without_index in both `UniqueWithoutIndexConstraints` and `Mutations` slice.

nit: long line

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_constraint.go line 439 at r20 (raw file):

}

// getIndexIDForValidationForConstraint returns the index ID this check constraint is supposed to

nit: long line

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 346 at r19 (raw file):

	}
	if op.ConstraintID >= tbl.NextConstraintID {
		tbl.NextConstraintID = op.ConstraintID + 1

why are you doing this in this function but not the others?

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 352 at r19 (raw file):

		TableID:      op.TableID,
		ColumnIDs:    op.ColumnIDs,
		Name:         tabledesc.ConstraintNamePlaceholder(op.ConstraintID),

why not use the constraint name?

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 385 at r19 (raw file):

			// Remove the mutation from the mutation slice. The `MakeMutationComplete`
			// call will also mark the above added unique_without_index as VALIDATED.
			// If this is a rollback of a drop, we are trying to add the unique_without_index constraint

nit: long line

pkg/sql/schemachanger/scexec/scmutationexec/constraint.go line 410 at r19 (raw file):

}

func (m *visitor) MakePublicUniqueWithoutIndexConstraintValidated(

The name of this function doesn't seem to match what it's doing

pkg/sql/schemachanger/scop/mutation.go line 369 at r19 (raw file):

}

// MakeAbsentUniqueWithoutIndexConstraintWriteOnly adds a non-existent unique_without_index constraint

nit: long line

pkg/sql/schemachanger/scpb/elements.proto line 325 at r19 (raw file):

  // Predicate, if non-nil, means a partial uniqueness constraint.
  Expression predicate = 4 [(gogoproto.customname) = "Predicate"];
  // IndexIDForValidation is the index id to hint to the unique_without_index constraint validation SQL query about which index

nit: long line

pkg/sql/schemachanger/scplan/internal/opgen/opgen_unique_without_index_constraint.go line 19 at r19 (raw file):

func init() {
	opRegistry.register((*scpb.UniqueWithoutIndexConstraint)(nil),

I'm not familiar enough with how this works to review this part

pkg/sql/schemachanger/scplan/internal/rules/testdata/deprules line 1447 at r22 (raw file):

    - $next-node[CurrentStatus] = TRANSIENT_DELETE_ONLY
    - descriptorIsNotBeingDropped($prev)
    - joinTargetNode($prev, $prev-target, $prev-node)

I don't know enough about this to review

Xiang-Gu · 2022-12-22T19:29:22Z

With this PR have you added support for all aspects of UNIQUE WITHOUT INDEX or just some? e.g., does this cover adding NOT VALID constraints and/or running VALIDATE on an existing constraint? How about adding them as part of a CREATE TABLE statement or dropping them?

@rytaft Thank you first of all for taking a look at this PR. To answer your question:

This PR does not support NOT VALID. This is a to-do for the team for all kinds of constraints.
We don't support CREATE TABLE in the declarative schema changer yet, so we cannot yet add them as part of a CREATE TABLE.
We don't support DROP CONSTRAINT in the declarative schema changer yet, so we cannot yet drop them in the declarative schema changer. We should (?) be able to drop them in the legacy schema changer though.
Another aspect is ADD COLUMN UNIQUE WITHOUT INDEX. We already support ADD COLUMN in the declarative schema changer. With this PR, I think we can easily add a bit to ADD COLUMN logic to support ADD COLUMN UNIQUE WITHOUT INDEX, which is currently unsupported with the following workaround

ERROR: adding a column marked as UNIQUE WITHOUT INDEX is unsupported
SQLSTATE: 0A000
HINT: add the column first, then run ALTER TABLE ... ADD CONSTRAINT to add a UNIQUE WITHOUT INDEX constraint on the column

Also seems like the change to include indexIDForValidation is mixed in with this change but should probably be its own separate change. It doesn't seem related to this.
This will be the key ingredient to support ADD COLUMN UNIQUE WITHOUT INDEX in the declarative schema changer. Btw, this is something required for (almost all) another constraint.

I will address your code review feedback shortly!

Xiang-Gu

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

pkg/sql/catalog/rewrite/rewrite.go line 176 at r21 (raw file):