sql: Alter Column Set Data Type General #46933

RichardJCai · 2020-04-02T16:48:12Z

This PR contains support for the ALTER TABLE ... ALTER COLUMN ... TYPE ... command.

To ensure this PR isn't too large, this PR only contains support for ALTER COLUMN TYPE for columns that are not part of indexes, do not own sequences, do not have constraints, and does not support using an expression. These features will come in future PRs.

Enable experimental ALTER COLUMN TYPE general using SET enable_experimental_alter_column_type_general = true;

Release note (sql change): Support for ALTER COLUMN TYPE online schema change for changing a column type's data.

cockroach-teamcity · 2020-04-02T16:48:26Z

This change is

pkg/sql/alter_table.go

pkg/sql/sqlbase/structured.go

pkg/sql/sqlbase/structured.proto

blathers-crl · 2020-04-08T21:25:22Z

❌ The GitHub CI (Cockroach) build has failed on a24c28ef.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan.}

blathers-crl · 2020-04-13T15:36:24Z

❌ The GitHub CI (Cockroach) build has failed on 5da66db9.

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

blathers-crl · 2020-04-13T22:18:34Z

❌ The GitHub CI (Cockroach) build has failed on 65d1bd86.

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

blathers-crl · 2020-04-14T01:07:42Z

❌ The GitHub CI (Cockroach) build has failed on c68de434.

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

blathers-crl · 2020-04-14T03:32:13Z

❌ The GitHub CI (Cockroach) build has failed on 4b89b608.

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

blathers-crl · 2020-04-14T16:42:30Z

❌ The GitHub CI (Cockroach) build has failed on f5fdad9e.

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

blathers-crl · 2020-04-14T16:54:20Z

❌ The GitHub CI (Cockroach) build has failed on 65f2b76e.

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

blathers-crl · 2020-04-14T18:29:06Z

❌ The GitHub CI (Cockroach) build has failed on ad01df4f.

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

blathers-crl · 2020-04-14T19:06:28Z

❌ The GitHub CI (Cockroach) build has failed on 37f5f01f.

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

rohany

Nice work!

It seems like you've included an update to yarn-vendor here while rebasing.

Some general comments/things I want to see added:

Feature flags / variable settings to enable use of this case
Version gating
More testing

pkg/sql/alter_column_type.go

pkg/sql/sqlbase/structured.proto

pkg/sql/alter_column_type_test.go

pkg/sql/alter_column_type.go

pkg/sql/alter_column_type_test.go

pkg/sql/logictest/testdata/logic_test/alter_column_type

rohany · 2020-04-14T20:50:27Z

I'm going to loop in the SQL Schema team as well.

jordanlewis

nit: commit messages and PRs should have a package prefix and have a summary of the change. Example:

sql: add experimental support for non-trivial ALTER COLUMN TYPE

Dismissed @rohany from 110 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @RichardJCai)

pkg/sql/alter_column_type.go, line 131 at r18 (raw file):

	if !params.p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.VersionAlterColumnTypeGeneral) {
		return pgerror.Newf(pgcode.FeatureNotSupported,
			"all nodes are not the correct version for alter column type general")

nit: "version %v must be finalized to run this alter column type"

pkg/sql/alter_column_type.go, line 135 at r18 (raw file):

	if !params.SessionData().AlterColumnTypeGeneral {
		return pgerror.Newf(pgcode.FeatureNotSupported,
			"alter column type general is experimental; "+

I don't think users will understand what "alter column type general" means. How about:

"ALTER TYPE %v is only supported experimentally". And the call to action should go in a hint. Check out how the temp tables one is done in create_table.go

pkg/sql/alter_column_type.go, line 139 at r18 (raw file):

	}
	// Disallow ALTER COLUMN ... TYPE ... USING EXPRESSION.
	// Todo(richardjcai): Todo, need to handle "inverse" expression

nit: Todos should be of the form TODO(richardjcai):

pkg/sql/alter_column_type.go, line 164 at r18 (raw file):

	// Disallow ALTER COLUMN TYPE general for columns that are
	// part of indexes.
	for i := range tableDesc.Indexes {

Use AllNonDropIndexes() to avoid repeating code for the indexes and prmary index.

pkg/sql/alter_column_type.go, line 226 at r18 (raw file):

				return err
			}
			newDefaultComputedExpr := tree.CastExpr{Expr: expr, Type: t.ToType, SyntaxMode: tree.CastShort}

It would be better to evaluate the cast expression up front, so it doesn't have to get evaluated once per row at backfill time. You can run .Eval() on it, since default expressions are always constant.

pkg/sql/alter_column_type_test.go, line 44 at r18 (raw file):

			RunBeforeChildJobs: func() {
				doOnce.Do(func() {
					fmt.Println("Once")

nit: This seems like a debug print.

pkg/sql/exec_util.go, line 229 at r18 (raw file):

var experimentalAlterColumnTypeGeneralMode = settings.RegisterBoolSetting(
	"sql.defaults.experimental_experimental_alter_column_type.enabled",

This has two experimentals in it.

pkg/sql/exec_util.go, line 230 at r18 (raw file):

var experimentalAlterColumnTypeGeneralMode = settings.RegisterBoolSetting(
	"sql.defaults.experimental_experimental_alter_column_type.enabled",
	"default value for experimental_alter_column_type session setting; enables a the use of Alter Column Type for general conversions",

This should say "enables the use of ALTER COLUMN TYPE for general conversions"

pkg/sql/logictest/testdata/logic_test/alter_column_type, line 338 at r18 (raw file):

CREATE TABLE t12 (x INT check (x > 0))

statement error pq: unimplemented: alter column type for a column that has a constraint is currently not supported

Can you add some tests for:

changing a column type that's included in the STORING columns of a secondary index
changing a column type that's part of a column an an interleaved table
collated strings?

pkg/sql/sessiondata/session_data.go, line 127 at r18 (raw file):

	// AlterColumnTypeGeneral is true if ALTER TABLE ... ALTER COLUMN ... TYPE x
	// may be used for general conversions requiring online schema change/
	AlterColumnTypeGeneral bool

add Enabled to the end of this

thoszhang

@RichardJCai Does this allow ALTER COLUMN TYPE to occur in the same transaction as other schema changes? It looks like you can't do it after other mutations have been queued, but can you queue other mutations after a column swap mutation has been queued? We should have tests for both of these cases.

@rohany In an attempt to mark the >100 Github-originating comments as resolved in Reviewable, I clicked the button to "dismiss" you from the conversation. I think this did the right thing in the Reviewable UI, but let me know if this caused problems.

Reviewed 1 of 26 files at r14, 1 of 17 files at r15, 3 of 17 files at r16, 17 of 22 files at r17, 4 of 4 files at r18.
Dismissed @rohany from 110 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/sql/alter_column_type.go, line 108 at r18 (raw file):

			return err
		}
		if err := params.p.createOrUpdateSchemaChangeJob(params.ctx, tableDesc, "alter column type", tableDesc.ClusterVersion.NextMutationID); err != nil {

This should have the actual ALTER TABLE statement.

pkg/sql/alter_column_type.go, line 113 at r18 (raw file):

		params.p.SendClientNotice(params.ctx, pgnotice.Newf("alter column type changes are finalized asynchronously; "+
			"further schema changes on this table may be restricted until the job completes; "+
			"some inserts into the altered column may be rejected until the schema change is finalized"))

nit: some writes to the altered column?

pkg/sql/alter_column_type.go, line 115 at r18 (raw file):

			"some inserts into the altered column may be rejected until the schema change is finalized"))
	default:
		return fmt.Errorf("unknown conversion for %s -> %s",

nit: This should be an assertion error, right?

pkg/sql/alter_column_type.go, line 131 at r18 (raw file):

	if !params.p.ExecCfg().Settings.Version.IsActive(params.ctx, clusterversion.VersionAlterColumnTypeGeneral) {
		return pgerror.Newf(pgcode.FeatureNotSupported,
			"all nodes are not the correct version for alter column type general")

nit: not all nodes?

pkg/sql/alter_column_type.go, line 151 at r18 (raw file):

	}

	// Disallow ALTER COLUMN TYPE general for columns that have a constraint.

We should also check for foreign keys involving this column, right?

pkg/sql/backfill.go, line 1359 at r18 (raw file):

			case *sqlbase.DescriptorMutation_ComputedColumnSwap:
				return unimplemented.NewWithIssue(47537,
					"performing an ALTER TABLE ... ALTER COLUMN TYPE in the same txn as the table was created is not supported")

nit: in the same transaction in which the table was created

pkg/sql/err_count_test.go, line 66 at r18 (raw file):

}

func TestUnimplementedCounts(t *testing.T) {

Is this test actually specific to this feature, or is it meant to be for testing telemetry for unimplemented features in general? If the latter, we should keep this test and replace it with a different unimplemented feature.

pkg/sql/schema_changer.go, line 890 at r18 (raw file):

				}
				// If we performed MakeMutationComplete on a PrimaryKeySwap mutation, then we need to start
				// a job for the index deletion mutations that the primary key swap mutation added, if any.

This comment should also mention the column swap.

pkg/sql/logictest/testdata/logic_test/alter_column_type, line 280 at r18 (raw file):

            )

# Ensure default expressions are casted correctly.

Can you add a test for what happens if evaluating the new default expression fails?

RichardJCai · 2020-05-21T15:18:08Z

I've addressed the PR review comments by Jordan and Lucy.

@lucy-zhang

func TestUnimplementedCounts(t *testing.T) {
Is this test actually specific to this feature, or is it meant to be for testing telemetry for > unimplemented features in general? If the latter, we should keep this test and replace it with a different unimplemented feature.

This test seems to be testing unimplemented features in general, however I think it this is a somewhat flawed test if we have to replace it whenever we implement the feature that's being used to test it. Also there are telemetry logictests that cover this.

@RichardJCai Does this allow ALTER COLUMN TYPE to occur in the same transaction as other schema changes? It looks like you can't do it after other mutations have been queued, but can you queue other mutations after a column swap mutation has been queued? We should have tests for both of these cases.

I've also disabled this ALTER COLUMN TYPE in explicit transactions in general since it's pretty hard to handle without transactional schema changes. After performing ALTER COLUMN TYPE in a transaction, any schema change that references the column will reference the old column, ie a constraint will be added to the old column, then the old column will be dropped and the constraint will apply to a dropped column after the swap. Created this issue to track this #49351.

thoszhang

This test seems to be testing unimplemented features in general, however I think it this is a somewhat flawed test if we have to replace it whenever we implement the feature that's being used to test it. Also there are telemetry logictests that cover this.

That may be true, but I'd rather not remove the test in this PR.

I've also disabled this ALTER COLUMN TYPE in explicit transactions in general since it's pretty hard to handle without transactional schema changes. After performing ALTER COLUMN TYPE in a transaction, any schema change that references the column will reference the old column, ie a constraint will be added to the old column, then the old column will be dropped and the constraint will apply to a dropped column after the swap. Created this issue to track this #49351.

That makes sense. What about something like alter table t alter column a set not null, alter column a set data type int;?

Reviewed 15 of 15 files at r19.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/clusterversion/cockroach_versions.go, line 498 at r19 (raw file):

	{
		// VersionAlterColumnTypeGeneral enables the use of alter column type for
		// non-trivial conversions.

I might actually say conversions that require the column data to be rewritten or something.

pkg/sql/alter_column_type.go, line 108 at r18 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This should have the actual ALTER TABLE statement.

tree.AsStringWithFQNames(t, params.Ann())

pkg/sql/alter_column_type_test.go, line 214 at r18 (raw file):

}

func TestSchemaChangeBeforeAlterColumnType(t *testing.T) {

Did this get turned into a different test?

pkg/sql/backfill.go, line 1359 at r18 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: in the same transaction in which the table was created

That's not quite the same as what you have.

RichardJCai · 2020-05-26T15:23:38Z

That makes sense. What about something like alter table t alter column a set not null, alter column a set data type int;?

Just to clarify, are you saying this should be allowed? The case where the ALTER COLUMN TYPE is the last schema change in the transaction is alright but to do that, we would have to go and block schema changes affecting the column after the ALTER COLUMN TYPE in the transaction. I think this should be done in a different PR so we don't add to much more code to this one. What are your thoughts?

RichardJCai · 2020-05-26T16:38:47Z

This test seems to be testing unimplemented features in general, however I think it this is a somewhat flawed test if we have to replace it whenever we implement the feature that's being used to test it. Also there are telemetry logictests that cover this.

That may be true, but I'd rather not remove the test in this PR.

I've also disabled this ALTER COLUMN TYPE in explicit transactions in general since it's pretty hard to handle without transactional schema changes. After performing ALTER COLUMN TYPE in a transaction, any schema change that references the column will reference the old column, ie a constraint will be added to the old column, then the old column will be dropped and the constraint will apply to a dropped column after the swap. Created this issue to track this #49351.

That makes sense. What about something like alter table t alter column a set not null, alter column a set data type int;?

Reviewed 15 of 15 files at r19.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/clusterversion/cockroach_versions.go, line 498 at r19 (raw file):
	{
		// VersionAlterColumnTypeGeneral enables the use of alter column type for
		// non-trivial conversions.
I might actually say conversions that require the column data to be rewritten or something.

pkg/sql/alter_column_type.go, line 108 at r18 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

This should have the actual ALTER TABLE statement.

tree.AsStringWithFQNames(t, params.Ann())

pkg/sql/alter_column_type_test.go, line 214 at r18 (raw file):
}

func TestSchemaChangeBeforeAlterColumnType(t *testing.T) {
Did this get turned into a different test?

pkg/sql/backfill.go, line 1359 at r18 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: in the same transaction in which the table was created

That's not quite the same as what you have.

Re-added back the unimplemented test and TestSchemaChangeBeforeAlterColumnType test (wasn't supposed to remove the last one).

And addressed the other comments.

thoszhang

Reviewed 5 of 5 files at r20, 5 of 5 files at r21.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

pkg/sql/alter_column_type.go, line 49 at r21 (raw file):

var alterColTypeInCombinationNotSupportedErr = unimplemented.NewWithIssuef(
	49351, "ALTER COLUMN TYPE cannot be used in combination "+
		"with other ALTER TABLE statements")

This should be commands instead of statements

Support experimental ALTER COLUMN TYPE general online schema change. Enable experimental ALTER COLUMN TYPE general using SET enable_experimental_alter_column_type_general = true; Release note (sql change): Added experimental support for ALTER COLUMN TYPE online schema change for changing a column type's data.

RichardJCai · 2020-05-27T23:06:49Z

bors r+

RichardJCai · 2020-05-27T23:07:22Z

Thanks for the reviews @rohany @lucy-zhang @jordanlewis!

craig · 2020-05-28T00:04:58Z

Build succeeded

GitHub CI (Cockroach)

ericharmeling · 2020-08-05T18:38:21Z

@RichardJCai
Is the plan to keep the enable_experimental_alter_column_type_general setting for 20.2 GA?

RichardJCai · 2020-08-05T18:52:33Z

@RichardJCai
Is the plan to keep the enable_experimental_alter_column_type_general setting for 20.2 GA?

Yeah that's the plan

knz · 2020-08-05T20:52:01Z

Would it be possible to drop the "experimental" word from the variable name, and only highlight its experimental nature in the description string and in docs? That will ensure that later users don't need to change their app code when we promote the feature to non experimental..

…

-- Verstuurd vanaf mijn Android apparaat met K-9 Mail. Excuseer mijn beknoptheid.

RichardJCai requested review from jordanlewis, rohany and a team April 2, 2020 16:48

rohany suggested changes Apr 2, 2020

View reviewed changes

RichardJCai force-pushed the alter_column_set_data_type branch from a24c28e to 5da66db Compare April 13, 2020 15:19

RichardJCai force-pushed the alter_column_set_data_type branch from 65d1bd8 to c68de43 Compare April 14, 2020 00:53

RichardJCai requested a review from a team April 14, 2020 00:53

RichardJCai requested review from a team as code owners April 14, 2020 00:53

RichardJCai force-pushed the alter_column_set_data_type branch from c68de43 to 458a613 Compare April 14, 2020 02:29

RichardJCai removed request for a team April 14, 2020 02:32

RichardJCai force-pushed the alter_column_set_data_type branch from 458a613 to 4b89b60 Compare April 14, 2020 02:36

RichardJCai force-pushed the alter_column_set_data_type branch from 4b89b60 to f5fdad9 Compare April 14, 2020 16:37

RichardJCai force-pushed the alter_column_set_data_type branch from f5fdad9 to 65f2b76 Compare April 14, 2020 16:49

RichardJCai force-pushed the alter_column_set_data_type branch from 65f2b76 to ad01df4 Compare April 14, 2020 18:26

RichardJCai force-pushed the alter_column_set_data_type branch from ad01df4 to 37f5f01 Compare April 14, 2020 18:39

rohany suggested changes Apr 14, 2020

View reviewed changes

RichardJCai force-pushed the alter_column_set_data_type branch from 52cbbec to 7df49a0 Compare May 13, 2020 22:06

jordanlewis reviewed May 19, 2020

View reviewed changes

thoszhang reviewed May 19, 2020

View reviewed changes

RichardJCai force-pushed the alter_column_set_data_type branch 3 times, most recently from 0fd1ffa to edf46bd Compare May 21, 2020 14:53

RichardJCai force-pushed the alter_column_set_data_type branch from edf46bd to 7705d9f Compare May 21, 2020 16:28

RichardJCai requested review from thoszhang, jordanlewis and rohany May 21, 2020 19:07

RichardJCai mentioned this pull request May 22, 2020

sql: Alter Column Type Using Expression #49452

Merged

thoszhang reviewed May 25, 2020

View reviewed changes

RichardJCai force-pushed the alter_column_set_data_type branch from 7705d9f to 0ca4b2e Compare May 26, 2020 16:37

RichardJCai force-pushed the alter_column_set_data_type branch 2 times, most recently from 930cf4c to a3e0c75 Compare May 26, 2020 20:18

thoszhang approved these changes May 27, 2020

View reviewed changes

RichardJCai force-pushed the alter_column_set_data_type branch from a3e0c75 to 9167cf0 Compare May 27, 2020 19:47

RichardJCai force-pushed the alter_column_set_data_type branch from 9167cf0 to dcd2429 Compare May 27, 2020 21:54

craig bot merged commit f4a5cac into cockroachdb:master May 28, 2020

jseldess mentioned this pull request Jun 24, 2020

sql: Alter Column Set Data Type General cockroachdb/docs#7633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Alter Column Set Data Type General #46933

sql: Alter Column Set Data Type General #46933

RichardJCai commented Apr 2, 2020 •

edited

Loading

cockroach-teamcity commented Apr 2, 2020

blathers-crl bot commented Apr 8, 2020

blathers-crl bot commented Apr 13, 2020

blathers-crl bot commented Apr 13, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

rohany left a comment

rohany commented Apr 14, 2020

jordanlewis left a comment

thoszhang left a comment

RichardJCai commented May 21, 2020

thoszhang left a comment

RichardJCai commented May 26, 2020

RichardJCai commented May 26, 2020 •

edited

Loading

thoszhang left a comment

RichardJCai commented May 27, 2020

RichardJCai commented May 27, 2020

craig bot commented May 28, 2020

ericharmeling commented Aug 5, 2020

RichardJCai commented Aug 5, 2020

knz commented Aug 5, 2020 via email

sql: Alter Column Set Data Type General #46933

sql: Alter Column Set Data Type General #46933

Conversation

RichardJCai commented Apr 2, 2020 • edited Loading

cockroach-teamcity commented Apr 2, 2020

blathers-crl bot commented Apr 8, 2020

blathers-crl bot commented Apr 13, 2020

blathers-crl bot commented Apr 13, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

blathers-crl bot commented Apr 14, 2020

rohany left a comment

Choose a reason for hiding this comment

rohany commented Apr 14, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

RichardJCai commented May 21, 2020

thoszhang left a comment

Choose a reason for hiding this comment

RichardJCai commented May 26, 2020

RichardJCai commented May 26, 2020 • edited Loading

thoszhang left a comment

Choose a reason for hiding this comment

RichardJCai commented May 27, 2020

RichardJCai commented May 27, 2020

craig bot commented May 28, 2020

Build succeeded

ericharmeling commented Aug 5, 2020

RichardJCai commented Aug 5, 2020

knz commented Aug 5, 2020 via email

RichardJCai commented Apr 2, 2020 •

edited

Loading

RichardJCai commented May 26, 2020 •

edited

Loading