Merge cockroachdb#79998 cockroachdb#80159

79998: sql: fix show create table after altering col type of col with comment r=chengxiong-ruan a=chengxiong-ruan

Previously `SHOW CREATE TABLE` use `sub_id` from `system.comments`
as a column id to find a column. However we actually use `PGAttributeNum`
when creating a column comment. So things will break if a column's id
has changed such as when type is altered.

Release note (sql change): Previously, if a column in a table has a comment,
`SHOW CREATE TABLE` would fail after the column type is changed.
Now this is fixed.

80159: Revert "sqlsmith: skip crdb_internal.reset_multi_region_zone_configs_for_database" r=rytaft a=rytaft

This reverts commit c1a959a.
That commit was unnecessary because
`crdb_internal.reset_multi_region_zone_configs_for_database` already
existed in the list of functions to skip.

Release note: None

Co-authored-by: Chengxiong Ruan <[email protected]>
Co-authored-by: Rebecca Taft <[email protected]>

docs/generated/redact_safe.md

@@ -42,6 +42,7 @@ pkg/sql/sem/catid/ids.go | `ConstraintID`
 pkg/sql/sem/catid/ids.go | `DescID`
 pkg/sql/sem/catid/ids.go | `FamilyID`
 pkg/sql/sem/catid/ids.go | `IndexID`
+pkg/sql/sem/catid/ids.go | `PGAttributeNum`
 pkg/sql/sqlliveness/sqlliveness.go | `SessionID`
 pkg/storage/enginepb/mvcc.go | `TxnEpoch`
 pkg/storage/enginepb/mvcc.go | `TxnSeq`

pkg/ccl/schemachangerccl/testdata/decomp/multiregion

@@ -69,11 +69,13 @@ ElementState:
   Status: PUBLIC
 - Column:
     columnId: 1
+    pgAttributeNum: 1
     tableId: 110
   Status: PUBLIC
 - Column:
     columnId: 2
     isHidden: true
+    pgAttributeNum: 2
     tableId: 110
   Status: PUBLIC
 - PrimaryIndex:
@@ -189,11 +191,13 @@ ElementState:
   Status: PUBLIC
 - Column:
     columnId: 1
+    pgAttributeNum: 1
     tableId: 109
   Status: PUBLIC
 - Column:
     columnId: 2
     isHidden: true
+    pgAttributeNum: 2
     tableId: 109
   Status: PUBLIC
 - PrimaryIndex:
@@ -311,15 +315,18 @@ ElementState:
   Status: PUBLIC
 - Column:
     columnId: 1
+    pgAttributeNum: 1
     tableId: 108
   Status: PUBLIC
 - Column:
     columnId: 2
+    pgAttributeNum: 2
     tableId: 108
   Status: PUBLIC
 - Column:
     columnId: 3
     isHidden: true
+    pgAttributeNum: 3
     tableId: 108
   Status: PUBLIC
 - PrimaryIndex:

pkg/ccl/schemachangerccl/testdata/decomp/partitioning

@@ -35,14 +35,17 @@ ElementState:
   Status: PUBLIC
 - Column:
     columnId: 1
+    pgAttributeNum: 1
     tableId: 104
   Status: PUBLIC
 - Column:
     columnId: 2
+    pgAttributeNum: 2
     tableId: 104
   Status: PUBLIC
 - Column:
     columnId: 3
+    pgAttributeNum: 3
     tableId: 104
   Status: PUBLIC
 - PrimaryIndex:
@@ -217,10 +220,12 @@ ElementState:
   Status: PUBLIC
 - Column:
     columnId: 1
+    pgAttributeNum: 1
     tableId: 105
   Status: PUBLIC
 - Column:
     columnId: 2
+    pgAttributeNum: 2
     tableId: 105
   Status: PUBLIC
 - PrimaryIndex:

pkg/ccl/schemachangerccl/testdata/end_to_end/drop_multiregion

@@ -197,6 +197,7 @@ upsert descriptor #108
   +    - elementProto:
   +        column:
   +          columnId: 1
+  +          pgAttributeNum: 1
   +          tableId: 108
   +      metadata:
   +        sourceElementId: 1
@@ -238,6 +239,7 @@ upsert descriptor #108
   +        column:
   +          columnId: 2
   +          isHidden: true
+  +          pgAttributeNum: 2
   +          tableId: 108
   +      metadata:
   +        sourceElementId: 1
@@ -537,6 +539,7 @@ upsert descriptor #108
   -    - elementProto:
   -        column:
   -          columnId: 1
+  -          pgAttributeNum: 1
   -          tableId: 108
   -      metadata:
   -        sourceElementId: 1
@@ -578,6 +581,7 @@ upsert descriptor #108
   -        column:
   -          columnId: 2
   -          isHidden: true
+  -          pgAttributeNum: 2
   -          tableId: 108
   -      metadata:
   -        sourceElementId: 1
@@ -873,6 +877,7 @@ upsert descriptor #109
   +    - elementProto:
   +        column:
   +          columnId: 1
+  +          pgAttributeNum: 1
   +          tableId: 109
   +      metadata:
   +        sourceElementId: 1
@@ -1106,6 +1111,7 @@ upsert descriptor #109
   -    - elementProto:
   -        column:
   -          columnId: 1
+  -          pgAttributeNum: 1
   -          tableId: 109
   -      metadata:
   -        sourceElementId: 1

pkg/internal/sqlsmith/schema.go

@@ -503,7 +503,6 @@ var functions = func() map[tree.FunctionClass]map[oid.Oid][]function {
 			"crdb_internal.start_replication_stream",
 			"crdb_internal.replication_stream_progress",
 			"crdb_internal.complete_replication_stream",
-			"crdb_internal.reset_multi_region_zone_configs_for_database",
 		} {
 			skip = skip || strings.Contains(def.Name, substr)
 		}

pkg/sql/catalog/colinfo/result_columns.go

@@ -60,7 +60,7 @@ func ResultColumnsFromColDescs(
 			Typ:            typ,
 			Hidden:         colDesc.Hidden,
 			TableID:        tableID,
-			PGAttributeNum: colDesc.GetPGAttributeNum(),
+			PGAttributeNum: uint32(colDesc.GetPGAttributeNum()),
 		}
 	}
 	return cols

pkg/sql/catalog/descpb/column.go

@@ -99,12 +99,12 @@ func (desc *ColumnDescriptor) CheckCanBeInboundFKRef() error {
 // GetPGAttributeNum returns the PGAttributeNum of the ColumnDescriptor
 // if the PGAttributeNum is set (non-zero). Returns the ID of the
 // ColumnDescriptor if the PGAttributeNum is not set.
-func (desc ColumnDescriptor) GetPGAttributeNum() uint32 {
+func (desc ColumnDescriptor) GetPGAttributeNum() PGAttributeNum {
 	if desc.PGAttributeNum != 0 {
 		return desc.PGAttributeNum
 	}
 
-	return uint32(desc.ID)
+	return PGAttributeNum(desc.ID)
 }
 
 // SQLStringNotHumanReadable returns the SQL statement describing the column.

pkg/sql/catalog/descpb/structured.go

@@ -119,6 +119,9 @@ const (
 	LatestIndexDescriptorVersion = PrimaryIndexWithStoredColumnsVersion
 )
 
+// PGAttributeNum is a custom type for ColumnDescriptor's PGAttributeNum field.
+type PGAttributeNum = catid.PGAttributeNum
+
 // ColumnID is a custom type for ColumnDescriptor IDs.
 type ColumnID = catid.ColumnID
 

pkg/sql/catalog/descpb/structured.proto

@@ -198,7 +198,9 @@ message ColumnDescriptor {
   // This only differs from ID when the Column order is swapped or
   // the ColumnDescriptor must be remade while remaining visual ordering.
   // This does not exist in TableDescriptors pre 20.2.
-  optional uint32 pg_attribute_num = 13 [(gogoproto.nullable) = false, (gogoproto.customname) = "PGAttributeNum"];
+  optional uint32 pg_attribute_num = 13 [(gogoproto.nullable) = false,
+                                         (gogoproto.customname) = "PGAttributeNum",
+                                         (gogoproto.casttype) = "PGAttributeNum"];
   // Used to indicate column is used and dropped for ALTER COLUMN TYPE mutation.
   optional bool alter_column_type_in_progress = 14 [(gogoproto.nullable) = false];
 

pkg/sql/catalog/descriptor.go

@@ -526,6 +526,12 @@ type TableDescriptor interface {
 	// provided target ID, in the canonical order.
 	// If no column is found then an error is also returned.
 	FindColumnWithID(id descpb.ColumnID) (Column, error)
+
+	// FindColumnWithPGAttributeNum returns the first column found whose
+	// PGAttributeNum (if set, otherwise ID) matches the provider id.
+	// Error is returned if no column is found.
+	FindColumnWithPGAttributeNum(id descpb.PGAttributeNum) (Column, error)
+
 	// FindColumnWithName returns the first column found whose name matches the
 	// provided target name, in the canonical order.
 	// If no column is found then an error is also returned.

pkg/sql/catalog/table_elements.go

@@ -332,7 +332,7 @@ type Column interface {
 	// GetPGAttributeNum returns the PGAttributeNum of the column descriptor
 	// if the PGAttributeNum is set (non-zero). Returns the ID of the
 	// column descriptor if the PGAttributeNum is not set.
-	GetPGAttributeNum() uint32
+	GetPGAttributeNum() descpb.PGAttributeNum
 
 	// IsSystemColumn returns true iff the column is a system column.
 	IsSystemColumn() bool

pkg/sql/catalog/tabledesc/column.go

@@ -195,7 +195,7 @@ func (w column) CheckCanBeOutboundFKRef() error {
 // GetPGAttributeNum returns the PGAttributeNum of the column descriptor
 // if the PGAttributeNum is set (non-zero). Returns the ID of the
 // column descriptor if the PGAttributeNum is not set.
-func (w column) GetPGAttributeNum() uint32 {
+func (w column) GetPGAttributeNum() descpb.PGAttributeNum {
 	return w.desc.GetPGAttributeNum()
 }
 

pkg/sql/catalog/tabledesc/structured_test.go

@@ -917,7 +917,7 @@ func TestLogicalColumnID(t *testing.T) {
 		actual := tests[i].desc.Columns[0].GetPGAttributeNum()
 		expected := tests[i].expected
 
-		if expected != actual {
+		if expected != uint32(actual) {
 			t.Fatalf("Expected PGAttributeNum to be %d, got %d.", expected, actual)
 		}
 	}

pkg/sql/catalog/tabledesc/table_desc.go

@@ -539,6 +539,19 @@ func (desc *wrapper) FindColumnWithID(id descpb.ColumnID) (catalog.Column, error
 	return nil, pgerror.Newf(pgcode.UndefinedColumn, "column-id \"%d\" does not exist", id)
 }
 
+// FindColumnWithPGAttributeNum implements the TableDescriptor interface.
+func (desc *wrapper) FindColumnWithPGAttributeNum(
+	id descpb.PGAttributeNum,
+) (catalog.Column, error) {
+	for _, col := range desc.AllColumns() {
+		if col.GetPGAttributeNum() == id {
+			return col, nil
+		}
+	}
+
+	return nil, pgerror.Newf(pgcode.UndefinedColumn, "column with logical order %q does not exist", id)
+}
+
 // FindColumnWithName implements the TableDescriptor interface.
 func (desc *wrapper) FindColumnWithName(name tree.Name) (catalog.Column, error) {
 	for _, col := range desc.AllColumns() {

pkg/sql/copy.go

@@ -214,7 +214,7 @@ func newCopyMachine(
 			Name:           col.GetName(),
 			Typ:            col.GetType(),
 			TableID:        tableDesc.GetID(),
-			PGAttributeNum: col.GetPGAttributeNum(),
+			PGAttributeNum: uint32(col.GetPGAttributeNum()),
 		}
 	}
 	// If there are no column specifiers and we expect non-visible columns

pkg/sql/indexbackfiller_test.go

@@ -298,7 +298,7 @@ INSERT INTO foo VALUES (1), (10), (100);
 					Nullable:       false,
 					DefaultExpr:    proto.String("42"),
 					Hidden:         false,
-					PGAttributeNum: uint32(mut.NextColumnID),
+					PGAttributeNum: descpb.PGAttributeNum(mut.NextColumnID),
 				}
 				mut.NextColumnID++
 				mut.AddColumnMutation(&columnWithDefault, descpb.DescriptorMutation_ADD)
@@ -311,7 +311,7 @@ INSERT INTO foo VALUES (1), (10), (100);
 					Nullable:       false,
 					ComputeExpr:    proto.String("i + def"),
 					Hidden:         false,
-					PGAttributeNum: uint32(mut.NextColumnID),
+					PGAttributeNum: descpb.PGAttributeNum(mut.NextColumnID),
 				}
 				mut.NextColumnID++
 				mut.AddColumnMutation(&computedColumnNotInPrimaryIndex, descpb.DescriptorMutation_ADD)

pkg/sql/logictest/testdata/logic_test/show_create

@@ -106,3 +106,37 @@ c  CREATE TABLE public.c (
 COMMENT ON TABLE public.c IS 'table';
 COMMENT ON COLUMN public.c.a IS 'column';
 COMMENT ON INDEX public.c@c_a_b_idx IS 'index'
+
+subtest alter_column_type_not_break_show_create
+
+statement ok
+SET enable_experimental_alter_column_type_general = true;
+
+statement ok
+CREATE TABLE t (c INT);
+
+statement ok
+COMMENT ON COLUMN t.c IS 'first comment';
+
+query T
+SELECT @2 FROM [SHOW CREATE TABLE t];
+----
+CREATE TABLE public.t (
+  c INT8 NULL,
+  rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
+  CONSTRAINT t_pkey PRIMARY KEY (rowid ASC)
+);
+COMMENT ON COLUMN public.t.c IS 'first comment'
+
+statement ok
+ALTER TABLE t ALTER COLUMN c TYPE character varying;
+
+query T
+SELECT @2 FROM [SHOW CREATE TABLE t];
+----
+CREATE TABLE public.t (
+  c VARCHAR NULL,
+  rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(),
+  CONSTRAINT t_pkey PRIMARY KEY (rowid ASC)
+);
+COMMENT ON COLUMN public.t.c IS 'first comment'