Merge #92289 #92597

92289: catalog,tabledesc: add & adopt Constraint subtypes r=Xiang-Gu a=postamar

These new interfaces help encapsulate the descriptor protobufs for table
constraints:
  - CheckConstraint,
  - ForeignKeyConstraint,
  - UniqueWithoutIndexConstraint,
  - UniqueWithIndexConstraint.

These are the leaves of the Constraint type tree, and Constraint is the
interface at the root of it. The changes in this commit mimic what was
done for Column & Index, which respectively wrap descpb.ColumnDescriptor
and descpb.IndexDescriptor.

These new constraint interfaces are adopted liberally throughout the
code base, but not completely: many references to descpb-protobufs still
need to be replaced.

This commit also removes the somewhat confusing concept of "active"
versus "inactive" constraint. Not only was it confusing to me, evidently
it was also confusing to other contributors considering how several mild
bugs and inconsistencies made their way into the code. This commit
replaces this concept with that of an "enforced" constraint:
  - A constraint is enforced if it applies to data written to the table,
    regardless of whether it has been validated on the data already in
    the table prior to the constraint coming into existence.

The implementation is somewhat awkward for constraints in that the same
constraint descriptor can be featured twice inside
a descpb.TableDescriptor, in its active constraint slice (such as
Checks, etc.) and also in the Mutations slice. In these cases the
interface wraps the non-mutation constraint descriptor protobuf, with no
observed changes to the database's behavior.

This commit required alterations to the table descriptor's
post-deserialization changes. The change which assigns constraint IDs
to table descriptors which don't have themhad some subtle bugs which went
unnoticed until this commit forced a heavier reliance on these fields.
This commit also adds a new change which ensures that a check
constraint's ColumnIDs slice is populated based on the columns
referenced in its expression, replacing ancient code which predates the
concept of post-deserialization changes and which would compute this
lazily.

As a more general observation: it appears that the handling of adding
and dropping constraints following other schema changes was mildly
inconsistent and this commit tries to improve this. For instance, it was
possible to drop a column which had been set as NOT NULL in the same
transaction, but not do the same for any other kind of constraint. Also
it was possible to reuse the name of a dropping constraint, despite this
constraint still being enforced. The new behavior is more strict, yet
this should be barely noticeable by the user in most practical cases:
it's rare that one wants to add a constraint referencing a column and
also drop that column in the same transaction, for instance.

Fixes #91918.

Release note: None

92597: upgrades: remove upgrade granting CREATELOGIN role opt r=andreimatei a=andreimatei

This patch removes a permanent upgrade that was granting the CREATELOGIN role option to users that had the CREATEROLE option. This upgrade was introduced in 20.2, meant to grant the then-new CREATELOGIN option to users created in 20.1 and prior that had the CREATEROLE option (CREATELOGIN was being split from CREATEROLE).

This code stayed around as a startupmigration since it was introduced, even though it didn't make sense for it to stay around after 20.2. Technically, I think this startup migration should have had the flag `IncludedInBootstrap=v20.2`, since we don't want it to run for clusters created at or after 20.2; this migration is not idempotent in the general sense, since it potentially picks up new, unintended users when it runs. Since 22.2, this migration would fail to run on anything but an empty system.role_options table because it would attempt to put NULLs into a non-nullable column. This was all benign since the startupmigrations had protection in place, preventing them for running a 2nd time after a completed attempt. So, for upgraded cluster, the migration only when the first 20.2 node came up, and for new clusters it would be a no-op since the system.role_options table starts empty.

This migration became problematic in #91627 when I've turned the startupmigrations into upgrades. These upgrades run once when upgrading to 23.1; I'm relying on their idempotence.

Fixes #92230
Fixes #92371
Fixes #92569

Release note: None
Epic: None

Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>

pkg/sql/alter_column_type.go

@@ -215,52 +215,31 @@ func alterColumnTypeGeneral(
 
 	// Disallow ALTER COLUMN TYPE general for columns that have a
 	// UNIQUE WITHOUT INDEX constraint.
-	for _, uc := range tableDesc.AllActiveAndInactiveUniqueWithoutIndexConstraints() {
-		for _, id := range uc.ColumnIDs {
-			if col.GetID() == id {
-				return colWithConstraintNotSupportedErr
-			}
+	for _, uc := range tableDesc.UniqueConstraintsWithoutIndex() {
+		if uc.CollectKeyColumnIDs().Contains(col.GetID()) {
+			return colWithConstraintNotSupportedErr
 		}
 	}
 
 	// Disallow ALTER COLUMN TYPE general for columns that have a foreign key
 	// constraint.
-	for _, fk := range tableDesc.AllActiveAndInactiveForeignKeys() {
-		if fk.OriginTableID == tableDesc.GetID() {
-			for _, id := range fk.OriginColumnIDs {
-				if col.GetID() == id {
-					return colWithConstraintNotSupportedErr
-				}
-			}
+	for _, fk := range tableDesc.OutboundForeignKeys() {
+		if fk.CollectOriginColumnIDs().Contains(col.GetID()) {
+			return colWithConstraintNotSupportedErr
 		}
-		if fk.ReferencedTableID == tableDesc.GetID() {
-			for _, id := range fk.ReferencedColumnIDs {
-				if col.GetID() == id {
-					return colWithConstraintNotSupportedErr
-				}
-			}
+		if fk.GetReferencedTableID() == tableDesc.GetID() &&
+			fk.CollectReferencedColumnIDs().Contains(col.GetID()) {
+			return colWithConstraintNotSupportedErr
 		}
 	}
 
 	// Disallow ALTER COLUMN TYPE general for columns that are
 	// part of indexes.
 	for _, idx := range tableDesc.NonDropIndexes() {
-		for i := 0; i < idx.NumKeyColumns(); i++ {
-			if idx.GetKeyColumnID(i) == col.GetID() {
-				return colInIndexNotSupportedErr
-			}
-		}
-		for i := 0; i < idx.NumKeySuffixColumns(); i++ {
-			if idx.GetKeySuffixColumnID(i) == col.GetID() {
-				return colInIndexNotSupportedErr
-			}
-		}
-		if !idx.Primary() {
-			for i := 0; i < idx.NumSecondaryStoredColumns(); i++ {
-				if idx.GetStoredColumnID(i) == col.GetID() {
-					return colInIndexNotSupportedErr
-				}
-			}
+		if idx.CollectKeyColumnIDs().Contains(col.GetID()) ||
+			idx.CollectKeySuffixColumnIDs().Contains(col.GetID()) ||
+			idx.CollectSecondaryStoredColumnIDs().Contains(col.GetID()) {
+			return colInIndexNotSupportedErr
 		}
 	}
 

pkg/sql/alter_primary_key.go

@@ -513,22 +513,14 @@ func (p *planner) AlterPrimaryKey(
 
 	// Determine if removing this index would lead to the uniqueness for a foreign
 	// key back reference, which will cause this swap operation to be blocked.
-	nonDropIndexes := tableDesc.NonDropIndexes()
-	remainingIndexes := make([]descpb.UniqueConstraint, 0, len(nonDropIndexes))
-	for i := range nonDropIndexes {
-		// We can't copy directly because of the interface conversion.
-		if nonDropIndexes[i].GetID() == tableDesc.GetPrimaryIndex().GetID() {
-			continue
-		}
-		remainingIndexes = append(remainingIndexes, nonDropIndexes[i])
-	}
-	remainingIndexes = append(remainingIndexes, newPrimaryIndexDesc)
+	const withSearchForReplacement = true
 	err = p.tryRemoveFKBackReferences(
 		ctx,
 		tableDesc,
 		tableDesc.GetPrimaryIndex(),
 		tree.DropRestrict,
-		remainingIndexes)
+		withSearchForReplacement,
+	)
 	if err != nil {
 		return err
 	}
@@ -737,6 +729,7 @@ func addIndexMutationWithSpecificPrimaryKey(
 ) error {
 	// Reset the ID so that a call to AllocateIDs will set up the index.
 	toAdd.ID = 0
+	toAdd.ConstraintID = 0
 	if err := table.AddIndexMutationMaybeWithTempIndex(toAdd, descpb.DescriptorMutation_ADD); err != nil {
 		return err
 	}