Merge #57146

57146: sql: use constraint name when adding a primary key constraint r=rafiss a=neeral

fixes #52833

Previously, when a table is created without a primary key and one
is later added by a named constraint, the specified name was
ignored. The primary key constraint always had the name "primary".

This was inadequate because the specified name was ignored. It
causes some trouble for schema migration tools.

To address this, this patch names the primary key as specified in
the `ALTER TABLE <t> ADD CONSTAINT <name> PRIMARY KEY` statement.

Release (sql change, bug fix): Adding a primary key constraint
to a table without a primary key no longer ignores the name
specified for the primary key.

Co-authored-by: neeral <8496732+neeral@users.noreply.github.com>

pkg/sql/alter_primary_key.go

@@ -129,6 +129,12 @@ func (p *planner) AlterPrimaryKey(
 		"new_primary_key",
 		nameExists,
 	)
+	if alterPKNode.Name != "" &&
+		// Allow reuse of existing primary key's name.
+		tableDesc.PrimaryIndex.Name != string(alterPKNode.Name) &&
+		nameExists(string(alterPKNode.Name)) {
+		return pgerror.Newf(pgcode.DuplicateObject, "constraint with name %s already exists", alterPKNode.Name)
+	}
 	newPrimaryIndexDesc := &descpb.IndexDescriptor{
 		Name:              name,
 		Unique:            true,
@@ -299,10 +305,11 @@ func (p *planner) AlterPrimaryKey(
 	}
 
 	swapArgs := &descpb.PrimaryKeySwap{
-		OldPrimaryIndexId: tableDesc.PrimaryIndex.ID,
-		NewPrimaryIndexId: newPrimaryIndexDesc.ID,
-		NewIndexes:        newIndexIDs,
-		OldIndexes:        oldIndexIDs,
+		OldPrimaryIndexId:   tableDesc.PrimaryIndex.ID,
+		NewPrimaryIndexId:   newPrimaryIndexDesc.ID,
+		NewIndexes:          newIndexIDs,
+		OldIndexes:          oldIndexIDs,
+		NewPrimaryIndexName: string(alterPKNode.Name),
 	}
 	tableDesc.AddPrimaryKeySwapMutation(swapArgs)
 

pkg/sql/alter_table.go

@@ -195,6 +195,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 						Columns:    d.Columns,
 						Sharded:    d.Sharded,
 						Interleave: d.Interleave,
+						Name:       d.Name,
 					}
 					if err := params.p.AlterPrimaryKey(params.ctx, n.tableDesc, alterPK); err != nil {
 						return err

pkg/sql/catalog/descpb/structured.proto

@@ -544,6 +544,10 @@ message PrimaryKeySwap {
   // swapped out with the i'th index in new_indexes.
   repeated uint32 old_indexes = 2 [(gogoproto.casttype) = "IndexID"];
   repeated uint32 new_indexes = 3 [(gogoproto.casttype) = "IndexID"];
+  // new_primary_index_name is the name of the primary key when added as a
+  // new constraint to a table without a primary key. In other cases, it is
+  // the empty string.
+  optional string new_primary_index_name = 5 [(gogoproto.nullable) = false];
 }
 
 // ComputedColumnSwap is a mutation corresponding to the atomic swap phase

pkg/sql/catalog/tabledesc/structured.go

@@ -3483,7 +3483,11 @@ func (desc *Mutable) MakeMutationComplete(m descpb.DescriptorMutation) error {
 			if err != nil {
 				return err
 			}
-			newIndex.Name = "primary"
+			if args.NewPrimaryIndexName == "" {
+				newIndex.Name = PrimaryKeyIndexName
+			} else {
+				newIndex.Name = args.NewPrimaryIndexName
+			}
 			desc.PrimaryIndex = *protoutil.Clone(newIndex).(*descpb.IndexDescriptor)
 			// The primary index "implicitly" stores all columns in the table.
 			// Explicitly including them in the stored columns list is incorrect.

pkg/sql/logictest/testdata/logic_test/alter_primary_key

@@ -726,17 +726,23 @@ t  CREATE TABLE public.t (
 statement ok
 DROP TABLE IF EXISTS t;
 CREATE TABLE t (x INT NOT NULL);
-ALTER TABLE t ADD CONSTRAINT "primary" PRIMARY KEY (x)
+ALTER TABLE t ADD CONSTRAINT "my_pk" PRIMARY KEY (x)
 
 query TT
 SHOW CREATE t
 ----
 t  CREATE TABLE public.t (
    x INT8 NOT NULL,
-   CONSTRAINT "primary" PRIMARY KEY (x ASC),
+   CONSTRAINT my_pk PRIMARY KEY (x ASC),
    FAMILY "primary" (x, rowid)
 )
 
+statement ok
+CREATE INDEX i ON t (x);
+
+statement error pq: constraint with name i already exists
+ALTER TABLE t DROP CONSTRAINT "my_pk", ADD CONSTRAINT "i" PRIMARY KEY (x);
+
 # Regression for #45362.
 statement ok
 DROP TABLE IF EXISTS t;

pkg/sql/sem/tree/alter_table.go

@@ -257,6 +257,7 @@ type AlterTableAlterPrimaryKey struct {
 	Columns    IndexElemList
 	Interleave *InterleaveDef
 	Sharded    *ShardedIndexDef
+	Name       Name
 }
 
 // TelemetryCounter implements the AlterTableCmd interface.