Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi and @Xiang-Gu)

pkg/sql/schemachanger/scexec/exec_backfill.go line 256 at r1 (raw file):

	}); err != nil {
		// We ran into an  uncategorized schema change error.
		deps.Telemetry().IncrementSchemaChangeErrorType("uncategorized")

so all we need to do is add a Telemetry interface for declarative schema changer, and invoke it during backfill if the backfill fails? I saw only the "uncategorized" error. Are there any other types of errors?

There's something funny going on in the restore case

Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, and @Xiang-Gu)

-- commits line 17 at r1:
detritus

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @Xiang-Gu)

-- commits line 17 at r1:

Done.

pkg/sql/schemachanger/scexec/exec_backfill.go line 256 at r1 (raw file):

The only other type we have is for constraint validation failures. These can be extended later on

This is very close, nice

Reviewed 2 of 9 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, @fqazi, and @Xiang-Gu)

pkg/sql/catalog/rewrite/rewrite.go line 397 at r3 (raw file):

}

// SchemaChangerStateInDescs goes over all mutable descriptors and cleans any

Can you expand this commentary to make it more clear what this does? I feel like the name could also be clearer. How about MaybeClearSchemaChangerStateInDescs? Explain why it exists and how it ought to be used.

Code quote:

// SchemaChangerStateInDescs goes over all mutable descriptors and cleans any
// empty state information.
func SchemaChangerStateInDescs(descriptors []catalog.MutableDescriptor) error {

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 170 at r3 (raw file):

	if desc.HasDefault() {
		expression := b.WrapExpression(tbl.TableID, cdd.DefaultExpr)
		// Sequence references inside expressions are unsupported, since these will

Is this no longer true?

pkg/sql/schemachanger/testdata/alter_table_add_column line 15 at r3 (raw file):

test
ALTER TABLE db.public.tbl ADD COLUMN j INT NOT NULL DEFAULT nextval('db.public.sq1')
----

nit: add a separate test

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @dt, and @Xiang-Gu)

pkg/sql/catalog/rewrite/rewrite.go line 397 at r3 (raw file):

// MaybeClearSchemaChangerStateInDescs goes over all mutable descriptors and
// cleans any state information from descriptors which have no targets associated
// with the corresponding jobs. Not having any targets in an individual state
// is not enough, since the descriptor in question could be a dependent (for
// example a type reference in a wider schema change).

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go line 170 at r3 (raw file):

Yes, this limitation is now lifted, we allowed sequence expressions inside default expressions.

pkg/sql/schemachanger/testdata/alter_table_add_column line 15 at r3 (raw file):

Done

Reviewed 1 of 4 files at r2, 6 of 9 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt, @fqazi, and @Xiang-Gu)

pkg/sql/schemachanger/testdata/alter_table_add_column line 2049 at r4 (raw file):

         - i
  -      name: crdb_internal_index_7_name_placeholder
  +      name: tbl_pkey

Should this still be called tbl_pkey here?

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner, @dt, and @Xiang-Gu)

pkg/sql/schemachanger/testdata/alter_table_add_column line 2049 at r4 (raw file):

That does look fishy, the existing rules have this issue. Let me handle this in the other PR where I'm adding UNIQUE support.