sql/schemachanger: support complex ALTER COLUMN … SET TYPE in DSC #133047

spilchen · 2024-10-21T12:21:16Z

Previously, complex column type changes that required a backfill reverted to the legacy schema changer. This update now enables support for such changes in the declarative schema changer (DSC).

As with the legacy process, a complex type change involves dropping the column being modified and adding a new one with the updated type. A temporary compute expression is used to map the old column to the new one for backfilling. The process includes performing a double backfill for the primary key (PK): first, both the old and new columns are included in the PK, and then a second backfill is done after the old column is removed. This staged approach is required due to a rule that prevents dropping the column’s compute expression before the dependent column is removed, keeping the expression until after the first backfill.

Since the old and new columns coexist temporarily, the old column is renamed to avoid ambiguity in the PK. A transient name is assigned to the dropped column (e.g., _shadow), which is automatically cleaned up later. This ensures clear differentiation during the transition. A new field was also added to ColumnName to restore the original name if the operation is undone, avoiding the use of a placeholder.

However, there are a few limitations with the current implementation:

No support for columns that already have a compute expression
No support for columns with ON UPDATE or DEFAULT expressions
No support for conversions to enum types
While column order is preserved in the catalog for SELECT * queries, the order within the column family is not maintained, with the new column being added at the end of the internal ordering.
Some tests, designed for the legacy schema changer, couldn’t easily be adapted for the DSC.

These limitations are planned to be addressed in future changes.

Epic: CRDB-40232
Closes #127014
Release notes: none

blathers-crl · 2024-10-21T12:21:20Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2024-10-21T12:21:25Z

This change is

rharding6373

Since this largely doesn't affect CDC I'm going to remove myself as a reviewer. Left one nit.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)

pkg/ccl/changefeedccl/cdcevent/event_test.go line 601 at r1 (raw file):

	// Use alter column type to force column reordering.
	sqlDB.Exec(t, `SET enable_experimental_alter_column_type_general = true`)
	// TODO(spilchen): force the legacy schema changer. When run with the DSC,

nit: use TODO(#133040)

annrpom

this is exciting! nice work; i just left some questions for my understanding

also:

The process includes performing a double backfill for the primary key (PK): first, both the old and new columns are included in the PK

could you elaborate why we need to include these columns in the PK?

let me ask the question about deprules in our -eng channel and then i'll stamp

annrpom · 2024-10-25T14:38:05Z

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_type.go

+	// Add the new column. It will be identical to the column it is replacing,
+	// except the type will differ, and it will have a transient computed expression.
+	// This expression will reference the original column to facilitate the backfill.
+	spec := addColumnSpec{


but the old column is still there until the first backfill what do you think if we say "set target status of old column to drop" and then in the new column text perhaps something like "after this backfill, the old column actually gets dropped"

i do think this is implied with your explanation further down so perhaps this is just a nit

annrpom · 2024-10-25T14:45:32Z

pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_name.go

 						ColumnID: this.ColumnID,
 						Name:     tabledesc.ColumnNamePlaceholder(this.ColumnID),
 					}
+					// If a name was provided for the transition to absent, override the placeholder.


for my understanding: is this just setting the new column to the column column's name?

annrpom · 2024-10-25T14:47:05Z

pkg/sql/schemachanger/scplan/internal/rules/current/dep_alter_column_type.go

 			}
 		},
 	)
+


ah i guess i had the same question about deprules as you did now :-); do we want to preserve 24.3's rules and then have this be in "current"?

annrpom

also: bazel extended ci failures seem related

Previously, complex column type changes that required a backfill reverted to the legacy schema changer. This update now enables support for such changes in the declarative schema changer (DSC). As with the legacy process, a complex type change involves dropping the column being modified and adding a new one with the updated type. A temporary compute expression is used to map the old column to the new one for backfilling. The process includes performing a double backfill for the primary key (PK): first, both the old and new columns are included in the PK, and then a second backfill is done after the old column is removed. This staged approach is required due to a rule that prevents dropping the column’s compute expression before the dependent column is removed, keeping the expression until after the first backfill. Since the old and new columns coexist temporarily, the old column is renamed to avoid ambiguity in the PK. A transient name is assigned to the dropped column (e.g., <col>_shadow), which is automatically cleaned up later. This ensures clear differentiation during the transition. A new field was also added to ColumnName to restore the original name if the operation is undone, avoiding the use of a placeholder. However, there are a few limitations with the current implementation: - No support for columns that already have a compute expression - No support for columns with ON UPDATE or DEFAULT expressions - No support for conversions to enum types - While column order is preserved in the catalog for SELECT * queries, the order within the column family is not maintained, with the new column being added at the end of the internal ordering. Some tests, designed for the legacy schema changer, couldn’t easily be adapted for the DSC. These limitations are planned to be addressed in future changes. Epic: CRDB-40232 Closes cockroachdb#127014 Release notes: none

rafiss

lgtm! nice work

Reviewed 43 of 48 files at r1, 1 of 2 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @spilchen)

spilchen · 2024-10-29T12:18:18Z

TFTR!

bors r+

craig · 2024-10-29T12:52:33Z

Build succeeded:

spilchen self-assigned this Oct 21, 2024

spilchen requested review from a team as code owners October 21, 2024 12:21

spilchen requested review from rharding6373 and removed request for a team October 21, 2024 12:21

spilchen force-pushed the issue-127014/general-conv-rev branch from bb18afc to e794b2c Compare October 21, 2024 13:09

rharding6373 reviewed Oct 21, 2024

View reviewed changes

spilchen force-pushed the issue-127014/general-conv-rev branch from e794b2c to 83ec1a4 Compare October 22, 2024 13:29

annrpom reviewed Oct 25, 2024

View reviewed changes

annrpom approved these changes Oct 25, 2024

View reviewed changes

spilchen force-pushed the issue-127014/general-conv-rev branch from 83ec1a4 to ed20701 Compare October 28, 2024 14:51

rafiss approved these changes Oct 28, 2024

View reviewed changes

craig bot merged commit ba425ba into cockroachdb:master Oct 29, 2024
22 of 23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

sql/schemachanger: support complex ALTER COLUMN … SET TYPE in DSC #133047

sql/schemachanger: support complex ALTER COLUMN … SET TYPE in DSC #133047

Uh oh!

spilchen commented Oct 21, 2024 •

edited

Loading

Uh oh!

blathers-crl bot commented Oct 21, 2024

Uh oh!

cockroach-teamcity commented Oct 21, 2024

Uh oh!

rharding6373 left a comment

Uh oh!

annrpom left a comment

Uh oh!

annrpom Oct 25, 2024

Uh oh!

annrpom Oct 25, 2024

Uh oh!

annrpom Oct 25, 2024

Uh oh!

annrpom left a comment •

edited

Loading

Uh oh!

rafiss left a comment

Uh oh!

spilchen commented Oct 29, 2024

Uh oh!

craig bot commented Oct 29, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sql/schemachanger: support complex ALTER COLUMN … SET TYPE in DSC #133047

sql/schemachanger: support complex ALTER COLUMN … SET TYPE in DSC #133047

Uh oh!

Conversation

spilchen commented Oct 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Oct 21, 2024

Uh oh!

cockroach-teamcity commented Oct 21, 2024

Uh oh!

rharding6373 left a comment

Choose a reason for hiding this comment

Uh oh!

annrpom left a comment

Choose a reason for hiding this comment

Uh oh!

annrpom Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

annrpom Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

annrpom Oct 25, 2024

Choose a reason for hiding this comment

Uh oh!

annrpom left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafiss left a comment

Choose a reason for hiding this comment

Uh oh!

spilchen commented Oct 29, 2024

Uh oh!

craig bot commented Oct 29, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spilchen commented Oct 21, 2024 •

edited

Loading

annrpom left a comment •

edited

Loading