sctestdeps: fix comments handling

This commit fixes a bug where comments weren't handled correctly by the
test state. This wasn't noticeable because the code wasn't actually
exercised anywhere. This commit also fixes this.

Release note: None

pkg/sql/schemachanger/scdeps/sctestdeps/test_deps.go

@@ -692,8 +692,9 @@ func (s *TestState) GetFullyQualifiedName(ctx context.Context, id descpb.ID) (st
 // NewCatalogChangeBatcher implements the scexec.Catalog interface.
 func (s *TestState) NewCatalogChangeBatcher() scexec.CatalogChangeBatcher {
 	return &testCatalogChangeBatcher{
-		s:             s,
-		namesToDelete: make(map[descpb.NameInfo]descpb.ID),
+		s:                s,
+		namesToDelete:    make(map[descpb.NameInfo]descpb.ID),
+		commentsToUpdate: make(map[catalogkeys.CommentKey]string),
 	}
 }
 
@@ -703,6 +704,7 @@ type testCatalogChangeBatcher struct {
 	namesToDelete       map[descpb.NameInfo]descpb.ID
 	descriptorsToDelete catalog.DescriptorIDSet
 	zoneConfigsToDelete catalog.DescriptorIDSet
+	commentsToUpdate    map[catalogkeys.CommentKey]string
 }
 
 var _ scexec.CatalogChangeBatcher = (*testCatalogChangeBatcher)(nil)
@@ -739,15 +741,15 @@ func (b *testCatalogChangeBatcher) DeleteZoneConfig(ctx context.Context, id desc
 func (b *testCatalogChangeBatcher) UpdateComment(
 	ctx context.Context, key catalogkeys.CommentKey, cmt string,
 ) error {
-	b.s.LogSideEffectf("upsert comment (objID: %d, subID %d, cmtType: %d, cmt: %s)", key.ObjectID, key.SubID, key.CommentType, cmt)
+	b.commentsToUpdate[key] = cmt
 	return nil
 }
 
 // DeleteComment implements the scexec.CatalogChangeBatcher interface.
 func (b *testCatalogChangeBatcher) DeleteComment(
 	ctx context.Context, key catalogkeys.CommentKey,
 ) error {
-	b.s.LogSideEffectf("delete all comments for (objID: %d, subID %d, cmtType: %d)", key.ObjectID, key.SubID, key.CommentType)
+	b.commentsToUpdate[key] = ""
 	return nil
 }
 
@@ -797,6 +799,28 @@ func (b *testCatalogChangeBatcher) ValidateAndRun(ctx context.Context) error {
 	for _, deletedID := range b.zoneConfigsToDelete.Ordered() {
 		b.s.LogSideEffectf("deleting zone config for #%d", deletedID)
 	}
+	commentKeys := make([]catalogkeys.CommentKey, 0, len(b.commentsToUpdate))
+	for key := range b.commentsToUpdate {
+		commentKeys = append(commentKeys, key)
+	}
+	sort.Slice(commentKeys, func(i, j int) bool {
+		if d := int(commentKeys[i].CommentType) - int(commentKeys[j].CommentType); d != 0 {
+			return d < 0
+		}
+		if d := int(commentKeys[i].ObjectID) - int(commentKeys[j].ObjectID); d != 0 {
+			return d < 0
+		}
+		return int(commentKeys[i].SubID)-int(commentKeys[j].SubID) < 0
+	})
+	for _, key := range commentKeys {
+		if cmt := b.commentsToUpdate[key]; cmt == "" {
+			b.s.LogSideEffectf("delete comment %s(objID: %d, subID: %d)",
+				key.CommentType, key.ObjectID, key.SubID)
+		} else {
+			b.s.LogSideEffectf("upsert comment %s(objID: %d, subID: %d) -> %q",
+				key.CommentType, key.ObjectID, key.SubID, cmt)
+		}
+	}
 	ve := b.s.uncommitted.Validate(
 		ctx,
 		clusterversion.TestingClusterVersion,

pkg/sql/schemachanger/scdeps/sctestdeps/test_state.go

@@ -127,6 +127,18 @@ func (s *TestState) WithTxn(fn func(s *TestState)) {
 			s.uncommitted.UpsertDescriptor(d)
 			return nil
 		})
+		_ = u.ForEachComment(func(key catalogkeys.CommentKey, cmt string) error {
+			s.committed.UpsertComment(key, cmt)
+			s.uncommitted.UpsertComment(key, cmt)
+			return nil
+		})
+		_ = u.ForEachZoneConfig(func(id catid.DescID, zc catalog.ZoneConfig) error {
+			zc = zc.Clone()
+			s.committed.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage())
+			zc = zc.Clone()
+			s.uncommitted.UpsertZoneConfig(id, zc.ZoneConfigProto(), zc.GetRawBytesInStorage())
+			return nil
+		})
 		s.LogSideEffectf("commit transaction #%d", s.txnCounter)
 		if len(s.createdJobsInCurrentTxn) > 0 {
 			s.LogSideEffectf("notified job registry to adopt jobs: %v", s.createdJobsInCurrentTxn)

pkg/sql/schemachanger/sctest/cumulative.go

@@ -666,9 +666,11 @@ func Rollback(t *testing.T, relPath string, newCluster NewClusterFunc) {
 	cumulativeTest(t, relPath, testFunc)
 }
 
+// fetchDescriptorStateQuery returns the CREATE statements for all descriptors
+// minus any COMMENT ON statements because these aren't consistently backed up.
 const fetchDescriptorStateQuery = `
 SELECT
-	create_statement
+	split_part(create_statement, ';', 1) AS create_statement
 FROM
 	( 
 		SELECT descriptor_id, create_statement FROM crdb_internal.create_schema_statements

pkg/sql/schemachanger/testdata/end_to_end/drop_column_basic

@@ -1,5 +1,7 @@
 setup
-CREATE TABLE t (i INT PRIMARY KEY, j INT)
+CREATE TABLE t (i INT PRIMARY KEY, j INT);
+COMMENT ON TABLE t IS 't has a comment';
+COMMENT ON COLUMN t.j IS 'j has a comment';
 ----
 ...
 +object {100 101 t} -> 104
@@ -37,7 +39,7 @@ begin transaction #1
 checking for feature: ALTER TABLE
 increment telemetry for sql.schema.alter_table
 increment telemetry for sql.schema.alter_table.drop_column
-## StatementPhase stage 1 of 1 with 7 MutationType ops
+## StatementPhase stage 1 of 1 with 9 MutationType ops
 upsert descriptor #104
   ...
          oid: 20
@@ -139,8 +141,9 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "1"
-  +  version: "2"
+  -  version: "2"
+  +  version: "3"
+delete comment ColumnCommentType(objID: 104, subID: 2)
 write *eventpb.AlterTable to event log: ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COLUMN ‹j›
 # end StatementPhase
 # begin PreCommitPhase
@@ -167,8 +170,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "1"
-  +  version: "2"
+  -  version: "2"
+  +  version: "3"
 create job #1 (non-cancelable: false): "ALTER TABLE defaultdb.public.t DROP COLUMN j"
   descriptor IDs: [104]
 # end PreCommitPhase
@@ -190,8 +193,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "2"
-  +  version: "3"
+  -  version: "3"
+  +  version: "4"
 update progress of schema change job #1: "PostCommitPhase stage 2 of 7 with 1 BackfillType op pending"
 commit transaction #3
 begin transaction #4
@@ -211,8 +214,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "3"
-  +  version: "4"
+  -  version: "4"
+  +  version: "5"
 update progress of schema change job #1: "PostCommitPhase stage 4 of 7 with 1 MutationType op pending"
 commit transaction #5
 begin transaction #6
@@ -228,8 +231,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "4"
-  +  version: "5"
+  -  version: "5"
+  +  version: "6"
 update progress of schema change job #1: "PostCommitPhase stage 5 of 7 with 1 BackfillType op pending"
 commit transaction #6
 begin transaction #7
@@ -249,8 +252,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "5"
-  +  version: "6"
+  -  version: "6"
+  +  version: "7"
 update progress of schema change job #1: "PostCommitPhase stage 7 of 7 with 1 ValidationType op pending"
 commit transaction #8
 begin transaction #9
@@ -355,8 +358,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "6"
-  +  version: "7"
+  -  version: "7"
+  +  version: "8"
 update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 2 of 3 with 2 MutationType ops pending"
 set schema change job #1 to non-cancellable
 commit transaction #10
@@ -402,8 +405,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "7"
-  +  version: "8"
+  -  version: "8"
+  +  version: "9"
 update progress of schema change job #1: "PostCommitNonRevertiblePhase stage 3 of 3 with 4 MutationType ops pending"
 commit transaction #11
 begin transaction #12
@@ -480,8 +483,8 @@ upsert descriptor #104
   ...
        time: {}
      unexposedParentSchemaId: 101
-  -  version: "8"
-  +  version: "9"
+  -  version: "9"
+  +  version: "10"
 write *eventpb.FinishSchemaChange to event log
 create job #2 (non-cancelable: true): "GC for ALTER TABLE defaultdb.public.t DROP COLUMN j"
   descriptor IDs: [104]

pkg/sql/schemachanger/testdata/end_to_end/drop_table

@@ -2,6 +2,7 @@ setup
 CREATE DATABASE db;
 CREATE SCHEMA db.sc;
 CREATE TABLE db.sc.t (k INT, v STRING);
+COMMENT ON TABLE db.sc.t IS 't has a comment';
 CREATE TYPE db.sc.e AS ENUM('a', 'b', 'c');
 ----
 ...
@@ -37,7 +38,7 @@ add synthetic descriptor #107:
      version: "1"
 # end StatementPhase
 # begin PreCommitPhase
-## PreCommitPhase stage 1 of 1 with 10 MutationType ops
+## PreCommitPhase stage 1 of 1 with 12 MutationType ops
 delete object namespace entry {104 106 t} -> 107
 upsert descriptor #107
   ...
@@ -65,6 +66,8 @@ upsert descriptor #107
      unexposedParentSchemaId: 106
   -  version: "1"
   +  version: "2"
+delete comment TableCommentType(objID: 107, subID: 0)
+write *eventpb.CommentOnTable to event log: DROP TABLE ‹db›.‹sc›.‹t›
 create job #1 (non-cancelable: true): "DROP TABLE db.sc.t"
   descriptor IDs: [107]
 # end PreCommitPhase

pkg/sql/schemachanger/testdata/explain/drop_column_basic

@@ -1,5 +1,7 @@
 /* setup */
 CREATE TABLE t (i INT PRIMARY KEY, j INT);
+COMMENT ON TABLE t IS 't has a comment';
+COMMENT ON COLUMN t.j IS 'j has a comment';
 
 /* test */
 EXPLAIN (ddl) ALTER TABLE t DROP COLUMN j;
@@ -14,13 +16,16 @@ Schema change plan for ALTER TABLE ‹defaultdb›.‹public›.‹t› DROP COL
  │         ├── 2 elements transitioning toward TRANSIENT_ABSENT
  │         │    ├── ABSENT → DELETE_ONLY   TemporaryIndex:{DescID: 104, IndexID: 3, ConstraintID: 3, SourceIndexID: 1}
  │         │    └── ABSENT → PUBLIC        IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 3}
- │         ├── 2 elements transitioning toward ABSENT
+ │         ├── 3 elements transitioning toward ABSENT
  │         │    ├── PUBLIC → WRITE_ONLY    Column:{DescID: 104, ColumnID: 2}
- │         │    └── PUBLIC → ABSENT        ColumnName:{DescID: 104, Name: j, ColumnID: 2}
- │         └── 7 Mutation operations
+ │         │    ├── PUBLIC → ABSENT        ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+ │         │    └── PUBLIC → ABSENT        ColumnComment:{DescID: 104, ColumnID: 2, Comment: j has a comment}
+ │         └── 9 Mutation operations
  │              ├── MakePublicColumnWriteOnly {"ColumnID":2,"TableID":104}
  │              ├── LogEvent {"TargetStatus":1}
  │              ├── SetColumnName {"ColumnID":2,"Name":"crdb_internal_co...","TableID":104}
+ │              ├── RemoveColumnComment {"ColumnID":2,"PgAttributeNum":2,"TableID":104}
+ │              ├── LogEvent {"TargetStatus":1}
  │              ├── MakeAbsentIndexBackfilling {"Index":{"ConstraintID":2,"IndexID":2,"IsUnique":true,"SourceIndexID":1,"TableID":104,"TemporaryIndexID":3}}
  │              ├── AddColumnToIndex {"ColumnID":1,"IndexID":2,"TableID":104}
  │              ├── MakeAbsentTempIndexDeleteOnly {"Index":{"ConstraintID":3,"IndexID":3,"IsUnique":true,"SourceIndexID":1,"TableID":104}}

pkg/sql/schemachanger/testdata/explain/drop_column_basic.rollback_1_of_7

@@ -1,5 +1,7 @@
 /* setup */
 CREATE TABLE t (i INT PRIMARY KEY, j INT);
+COMMENT ON TABLE t IS 't has a comment';
+COMMENT ON COLUMN t.j IS 'j has a comment';
 
 /* test */
 ALTER TABLE t DROP COLUMN j;
@@ -8,17 +10,20 @@ EXPLAIN (ddl) rollback at post-commit stage 1 of 7;
 Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j›; 
  └── PostCommitNonRevertiblePhase
       └── Stage 1 of 1 in PostCommitNonRevertiblePhase
-           ├── 2 elements transitioning toward PUBLIC
+           ├── 3 elements transitioning toward PUBLIC
            │    ├── WRITE_ONLY    → PUBLIC Column:{DescID: 104, ColumnID: 2}
-           │    └── ABSENT        → PUBLIC ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+           │    ├── ABSENT        → PUBLIC ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+           │    └── ABSENT        → PUBLIC ColumnComment:{DescID: 104, ColumnID: 2, Comment: j has a comment}
            ├── 5 elements transitioning toward ABSENT
            │    ├── BACKFILL_ONLY → ABSENT PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 2, TemporaryIndexID: 3, SourceIndexID: 1}
            │    ├── PUBLIC        → ABSENT IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 2}
            │    ├── PUBLIC        → ABSENT IndexData:{DescID: 104, IndexID: 2}
            │    ├── DELETE_ONLY   → ABSENT TemporaryIndex:{DescID: 104, IndexID: 3, ConstraintID: 3, SourceIndexID: 1}
            │    └── PUBLIC        → ABSENT IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 3}
-           └── 8 Mutation operations
+           └── 10 Mutation operations
                 ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
+                ├── UpsertColumnComment {"ColumnID":2,"Comment":"j has a comment","PGAttributeNum":2,"TableID":104}
+                ├── LogEvent {"TargetStatus":2}
                 ├── MakeWriteOnlyColumnPublic {"ColumnID":2,"TableID":104}
                 ├── RefreshStats {"TableID":104}
                 ├── MakeIndexAbsent {"IndexID":2,"TableID":104}

pkg/sql/schemachanger/testdata/explain/drop_column_basic.rollback_2_of_7

@@ -1,5 +1,7 @@
 /* setup */
 CREATE TABLE t (i INT PRIMARY KEY, j INT);
+COMMENT ON TABLE t IS 't has a comment';
+COMMENT ON COLUMN t.j IS 'j has a comment';
 
 /* test */
 ALTER TABLE t DROP COLUMN j;
@@ -8,16 +10,19 @@ EXPLAIN (ddl) rollback at post-commit stage 2 of 7;
 Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j›; 
  └── PostCommitNonRevertiblePhase
       ├── Stage 1 of 2 in PostCommitNonRevertiblePhase
-      │    ├── 2 elements transitioning toward PUBLIC
+      │    ├── 3 elements transitioning toward PUBLIC
       │    │    ├── WRITE_ONLY    → PUBLIC      Column:{DescID: 104, ColumnID: 2}
-      │    │    └── ABSENT        → PUBLIC      ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+      │    │    ├── ABSENT        → PUBLIC      ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+      │    │    └── ABSENT        → PUBLIC      ColumnComment:{DescID: 104, ColumnID: 2, Comment: j has a comment}
       │    ├── 4 elements transitioning toward ABSENT
       │    │    ├── BACKFILL_ONLY → ABSENT      PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 2, TemporaryIndexID: 3, SourceIndexID: 1}
       │    │    ├── PUBLIC        → ABSENT      IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 2}
       │    │    ├── WRITE_ONLY    → DELETE_ONLY TemporaryIndex:{DescID: 104, IndexID: 3, ConstraintID: 3, SourceIndexID: 1}
       │    │    └── PUBLIC        → ABSENT      IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 3}
-      │    └── 7 Mutation operations
+      │    └── 9 Mutation operations
       │         ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
+      │         ├── UpsertColumnComment {"ColumnID":2,"Comment":"j has a comment","PGAttributeNum":2,"TableID":104}
+      │         ├── LogEvent {"TargetStatus":2}
       │         ├── MakeWriteOnlyIndexDeleteOnly {"IndexID":3,"TableID":104}
       │         ├── MakeWriteOnlyColumnPublic {"ColumnID":2,"TableID":104}
       │         ├── RefreshStats {"TableID":104}

pkg/sql/schemachanger/testdata/explain/drop_column_basic.rollback_3_of_7

@@ -1,5 +1,7 @@
 /* setup */
 CREATE TABLE t (i INT PRIMARY KEY, j INT);
+COMMENT ON TABLE t IS 't has a comment';
+COMMENT ON COLUMN t.j IS 'j has a comment';
 
 /* test */
 ALTER TABLE t DROP COLUMN j;
@@ -8,16 +10,19 @@ EXPLAIN (ddl) rollback at post-commit stage 3 of 7;
 Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j›; 
  └── PostCommitNonRevertiblePhase
       ├── Stage 1 of 2 in PostCommitNonRevertiblePhase
-      │    ├── 2 elements transitioning toward PUBLIC
+      │    ├── 3 elements transitioning toward PUBLIC
       │    │    ├── WRITE_ONLY    → PUBLIC      Column:{DescID: 104, ColumnID: 2}
-      │    │    └── ABSENT        → PUBLIC      ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+      │    │    ├── ABSENT        → PUBLIC      ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+      │    │    └── ABSENT        → PUBLIC      ColumnComment:{DescID: 104, ColumnID: 2, Comment: j has a comment}
       │    ├── 4 elements transitioning toward ABSENT
       │    │    ├── BACKFILL_ONLY → ABSENT      PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 2, TemporaryIndexID: 3, SourceIndexID: 1}
       │    │    ├── PUBLIC        → ABSENT      IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 2}
       │    │    ├── WRITE_ONLY    → DELETE_ONLY TemporaryIndex:{DescID: 104, IndexID: 3, ConstraintID: 3, SourceIndexID: 1}
       │    │    └── PUBLIC        → ABSENT      IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 3}
-      │    └── 7 Mutation operations
+      │    └── 9 Mutation operations
       │         ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
+      │         ├── UpsertColumnComment {"ColumnID":2,"Comment":"j has a comment","PGAttributeNum":2,"TableID":104}
+      │         ├── LogEvent {"TargetStatus":2}
       │         ├── MakeWriteOnlyIndexDeleteOnly {"IndexID":3,"TableID":104}
       │         ├── MakeWriteOnlyColumnPublic {"ColumnID":2,"TableID":104}
       │         ├── RefreshStats {"TableID":104}

pkg/sql/schemachanger/testdata/explain/drop_column_basic.rollback_4_of_7

@@ -1,5 +1,7 @@
 /* setup */
 CREATE TABLE t (i INT PRIMARY KEY, j INT);
+COMMENT ON TABLE t IS 't has a comment';
+COMMENT ON COLUMN t.j IS 'j has a comment';
 
 /* test */
 ALTER TABLE t DROP COLUMN j;
@@ -8,16 +10,19 @@ EXPLAIN (ddl) rollback at post-commit stage 4 of 7;
 Schema change plan for rolling back ALTER TABLE ‹defaultdb›.public.‹t› DROP COLUMN ‹j›; 
  └── PostCommitNonRevertiblePhase
       ├── Stage 1 of 2 in PostCommitNonRevertiblePhase
-      │    ├── 2 elements transitioning toward PUBLIC
+      │    ├── 3 elements transitioning toward PUBLIC
       │    │    ├── WRITE_ONLY  → PUBLIC      Column:{DescID: 104, ColumnID: 2}
-      │    │    └── ABSENT      → PUBLIC      ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+      │    │    ├── ABSENT      → PUBLIC      ColumnName:{DescID: 104, Name: j, ColumnID: 2}
+      │    │    └── ABSENT      → PUBLIC      ColumnComment:{DescID: 104, ColumnID: 2, Comment: j has a comment}
       │    ├── 4 elements transitioning toward ABSENT
       │    │    ├── DELETE_ONLY → ABSENT      PrimaryIndex:{DescID: 104, IndexID: 2, ConstraintID: 2, TemporaryIndexID: 3, SourceIndexID: 1}
       │    │    ├── PUBLIC      → ABSENT      IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 2}
       │    │    ├── WRITE_ONLY  → DELETE_ONLY TemporaryIndex:{DescID: 104, IndexID: 3, ConstraintID: 3, SourceIndexID: 1}
       │    │    └── PUBLIC      → ABSENT      IndexColumn:{DescID: 104, ColumnID: 1, IndexID: 3}
-      │    └── 7 Mutation operations
+      │    └── 9 Mutation operations
       │         ├── SetColumnName {"ColumnID":2,"Name":"j","TableID":104}
+      │         ├── UpsertColumnComment {"ColumnID":2,"Comment":"j has a comment","PGAttributeNum":2,"TableID":104}
+      │         ├── LogEvent {"TargetStatus":2}
       │         ├── MakeWriteOnlyIndexDeleteOnly {"IndexID":3,"TableID":104}
       │         ├── MakeWriteOnlyColumnPublic {"ColumnID":2,"TableID":104}
       │         ├── RefreshStats {"TableID":104}