schemachanger: Implement `ALTER COLUMN ... SET NOT NULL` #95556

Xiang-Gu · 2023-01-19T22:35:14Z

This PR implements ALTER TABLE t ALTER COLUMN c SET NOT NULL in the declarative schema changer.

The key change is to introduce the ColumnNotNull element for column NOT NULL constraint. Previously, this is recorded in a bool field in the ColumnType element. We will treat ColumnNotNull in a similar fashion as CheckConstraint where we indeed uses a dummy check constraint for validation.

The plan to address mixed version compatibility is laid out in comments in commit 2.

Epic: None

cockroach-teamcity · 2023-01-19T22:35:22Z

This change is

pkg/sql/backfill.go

pkg/sql/catalog/tabledesc/column.go

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

pkg/sql/logictest/testdata/logic_test/alter_table

ajwerner

Nice, the TestXiang stuff, of course, should use some cleanup.

Can you add some end-to-end tests?

Xiang-Gu · 2023-01-24T14:40:25Z

@ajwerner I've addressed your early feedback. I'm investigating the one and last test failure in CI. At the same time, can you give it another look? Thank you a lot

Xiang-Gu · 2023-01-24T17:50:28Z

This is officially ready for review!

ajwerner

Very nice!

Reviewed 7 of 32 files at r1, 8 of 27 files at r2, 50 of 168 files at r3, 7 of 15 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Xiang-Gu)

pkg/sql/schemachanger/scdecomp/decomp.go line 449 at r4 (raw file):

			//ConstraintID: tbl.GetNextConstraintID(),
		})
		//tbl.TableDesc().NextConstraintID++

detritus?

Code quote:

			//ConstraintID: tbl.GetNextConstraintID(),
		})
		//tbl.TableDesc().NextConstraintID++
	}

pkg/sql/schemachanger/scexec/scmutationexec/helpers.go line 222 at r4 (raw file):

}

func enqueueAddNotNullMutation(

nit: this subtraction is getting silly, and nothing seems to return an error. We can use generics now. What if you do:

func addMutation[M any](f func(M, descpb.DescriptorMutation_Direction), m M, dir descpb.DescriptoMutation_Direction) {
    f(tbl, m, dir)
    tbl.NextMutationID--
}

Then you can call this with:

addMutation(tbl.AddNotNullMutation, ck, descpb.DescriptorMutation_ADD)

pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_set_not_null line 7 at r4 (raw file):

+object {100 101 t} -> 104

test

can you add some statements during this which attempt to write null values and fail at the appropriate stages?

postamar · 2023-01-24T21:59:20Z

Can you hold off before merging this please? I would like to get #95631 in first, because otherwise a lot of rebasing is needed on my end and time is running out before I go on leave. Thanks!

postamar

I did a pass, this approach seems sound.

postamar · 2023-01-25T18:10:16Z

pkg/sql/schemachanger/scplan/internal/rules/release_22_2/testdata/deprules

@@ -2023,7 +2120,7 @@ deprules
  to: idx-or-col-Node
  query:
    - $descriptor[Type] IN ['*scpb.Database', '*scpb.Schema', '*scpb.View', '*scpb.Sequence', '*scpb.Table', '*scpb.EnumType', '*scpb.AliasType', '*scpb.CompositeType']
-    - $idx-or-col[Type] IN ['*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint']
+    - $idx-or-col[Type] IN ['*scpb.Column', '*scpb.PrimaryIndex', '*scpb.SecondaryIndex', '*scpb.TemporaryIndex', '*scpb.UniqueWithoutIndexConstraint', '*scpb.CheckConstraint', '*scpb.ForeignKeyConstraint', '*scpb.ColumnNotNull']


None of the test output in the release_22_2 package should change, ever. Also, I don't think that foreign key constraints are featured in 22.2 so I filed an issue: #95849

postamar · 2023-01-25T18:22:48Z

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

+			))
+		}
+	})
+}


nit: Should this be in alter_table_add_constraint.go instead? I realize there's no actual ADD CONSTRAINT syntax for NOT NULL, so you be the judge.

postamar · 2023-01-25T18:23:04Z

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

@@ -0,0 +1,51 @@
+// Copyright 2022 The Cockroach Authors.


s/2022/2023

postamar · 2023-01-25T18:24:59Z

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

@@ -172,6 +172,7 @@ func init() {
 				(*scpb.CheckConstraint)(nil),
 				(*scpb.ForeignKeyConstraint)(nil),
 				(*scpb.UniqueWithoutIndexConstraint)(nil),
+				(*scpb.ColumnNotNull)(nil),


Perhaps we should have a filter in the helpers for non-index-backed constraints?

postamar · 2023-01-25T18:30:25Z

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

+	// "too late" (i.e. when column transitions to ABSENT) is that the
+	// constraint would still be visible and enforced while the column is not writable.
+	// This can cause concurrent WRITE to see and attempt to uphold the constraint,
+	// but it wouldn't be able to find the column.


Thanks for the explanation, I get why it needs to be in the same stage. Any reason why it's the not-null before the column and not the other way round? Not that it matters, just curious.

postamar · 2023-01-25T18:33:40Z

pkg/sql/schemachanger/scpb/elements.proto

+  //  - Delete `is_23_1_cluster_version_active` field from protobuf.
+  // This plan will guarantee compatibility in all possible mixed version states
+  // as well as restoring from a old backup.
+  bool deprecated_is_nullable = 5;


You can deprecate fields in protobuf with [deprecated=true]. Also add // Deprecated. in the comment block above for a pretty strike-through visual effect in GoLand.

postamar · 2023-01-25T18:36:47Z

pkg/sql/schemachanger/scpb/elements.proto

+  // V24_1:
+  //  - Delete `is_23_1_cluster_version_active` field from protobuf.
+  // This plan will guarantee compatibility in all possible mixed version states
+  // as well as restoring from a old backup.


Comments such as these don't typically age well. I recommend you use language which is more non-committal and less subject to current assumptions as to what the future will look like. You're far better off filing tickets in github to track future work.

postamar · 2023-01-25T18:42:28Z

pkg/sql/schemachanger/scpb/elements.proto

+  // This plan will guarantee compatibility in all possible mixed version states
+  // as well as restoring from a old backup.
+  bool deprecated_is_nullable = 5;
+  bool is_23_1_cluster_version_active = 11;


Consider renaming this to is_element_created_in_23_1_or_later? This field is a neat solution to the problem at hand, which I'd find mildly distasteful if it weren't for the fact that we need to migrate to a new model for ColumnType in this release anyway as per #92605

An alternative to consider, which might age better: define a

message ElementCreationMetadata { bool in_23_1_or_later = 1; }

and embed it into ColumnType. Then in scdecomp provide a helper which generates a scpb.ElementCreationMetadata instance based on the cluster settings. This way this logic can be re-used and adapted for later versions and for other elements as well.

Another alternative: have this metadata in the TargetMetadata message instead.

postamar · 2023-01-25T18:58:52Z

pkg/sql/schemachanger/scpb/elements.proto

+  //  - Start deprecating `is_nullable` (rename it to `deprecated_is_nullable`);
+  //  - Introduce `is_23_1_cluster_version_active`;
+  //  - Add a version gate in the builder for `ADD COLUMN` (see addColumn function)
+  //    to set `is_23_1_cluster_version_active` if cluster version V23_1 is active.


Shouldn't scdecomp be doing that as well?

Xiang-Gu

@ajwerner @postamar Thank you both for the feedback. I've addressed them all. I will hold off merging until #95631 is in, but in the meantime, I'd like to hear any other comments/feedback you still have for this PR.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_not_null.go line 1 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

s/2022/2023

Done.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_not_null.go line 51 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: Should this be in alter_table_add_constraint.go instead? I realize there's no actual ADD CONSTRAINT syntax for NOT NULL, so you be the judge.

I'd like to keep as is bc 1. it's less boilerplate code added (all the nested min supported version control for add constraints), and 2. we don't have SET NOT NULL NOT VALIDas other constraints.

pkg/sql/schemachanger/scdecomp/decomp.go line 449 at r4 (raw file):

Previously, ajwerner wrote…

detritus?

done

Code quote:

			ColumnID: col.GetID(),
			//ConstraintID: tbl.GetNextConstraintID(),

pkg/sql/schemachanger/scexec/scmutationexec/helpers.go line 222 at r4 (raw file):

Previously, ajwerner wrote…

nit: this subtraction is getting silly, and nothing seems to return an error. We can use generics now. What if you do:
func addMutation[M any](f func(M, descpb.DescriptorMutation_Direction), m M, dir descpb.DescriptoMutation_Direction) {
    f(tbl, m, dir)
    tbl.NextMutationID--
}
Then you can call this with:
addMutation(tbl.AddNotNullMutation, ck, descpb.DescriptorMutation_ADD)

This is a good idea! Done

pkg/sql/schemachanger/scpb/elements.proto line 209 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Comments such as these don't typically age well. I recommend you use language which is more non-committal and less subject to current assumptions as to what the future will look like. You're far better off filing tickets in github to track future work.

done, filed #95893

pkg/sql/schemachanger/scpb/elements.proto line 210 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

You can deprecate fields in protobuf with [deprecated=true]. Also add // Deprecated. in the comment block above for a pretty strike-through visual effect in GoLand.

done

pkg/sql/schemachanger/scpb/elements.proto line 211 at r3 (raw file):

message ElementCreationMetadata {
bool in_23_1_or_later = 1;
}
I like this idea. I did some plumbing so that we have the cluster version in scdecomp.

pkg/sql/schemachanger/scpb/elements.proto line 196 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Shouldn't scdecomp be doing that as well?

it's okay bc we have a minSupportedVersion on elements and any elements not supported in current cluster version will be filtered out. In fact, I think this makes it easier to think about: In the builder, we use the version gate to only set this newly added field is_23_1_cluster_version_active. We still decomp and add/drop newly introduced elements (in addition to what we've already been doing) while rest assured that it will be filtered out anyway if they're not supported.

pkg/sql/schemachanger/scplan/internal/rules/current/dep_drop_column.go line 115 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Thanks for the explanation, I get why it needs to be in the same stage. Any reason why it's the not-null before the column and not the other way round? Not that it matters, just curious.

I think that'd be fine. All we need here is same-stage.

pkg/sql/schemachanger/scplan/internal/rules/current/op_drop.go line 175 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Perhaps we should have a filter in the helpers for non-index-backed constraints?

We do! IsSupportedNonIndexBackedConstraint

Code quote:

			constraint.Type(
				(*scpb.CheckConstraint)(nil),
				(*scpb.ForeignKeyConstraint)(nil),
				(*scpb.UniqueWithoutIndexConstraint)(nil),
				(*scpb.ColumnNotNull)(nil),

pkg/sql/schemachanger/scplan/internal/rules/release_22_2/testdata/deprules line 2123 at r2 (raw file):

Previously, postamar (Marius Posta) wrote…

None of the test output in the release_22_2 package should change, ever. Also, I don't think that foreign key constraints are featured in 22.2 so I filed an issue: #95849

yeah, we are aware of this and thanks for filing the issue.

pkg/sql/schemachanger/testdata/end_to_end/alter_table_alter_column_set_not_null line 7 at r4 (raw file):

Previously, ajwerner wrote…

can you add some statements during this which attempt to write null values and fail at the appropriate stages?

Done!

pkg/sql/catalog/tabledesc/column.go line 37 at r1 (raw file):

Previously, ajwerner wrote…

this is detritus, right?

Done.

pkg/sql/catalog/tabledesc/validate.go

ajwerner

LGTM

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

This commit introduce a new element protobuf `ColumnNotNull`, which will be used in later commit to re-implement how we handle NOT NULL constraint of a column.

This commit uses the newly introduced element `ColumnNotNull` and re-implement logic handling NOT NULL constraint of a column in the declarative schema changer. The key idea is to treat NOT NULL as a functionally equivalent CHECK constraint, (col_name IS NOT NULL), and use existing validation logic for CHECK to validate the NOT NULL constraint. This commit is a implementation change for something we already support in the declarative schema changer, so the way we achieve compatibility in mixed version state is to properly version gate the builder and mutation execution. The detailed plan can be found in the comment inside `ColumnType` element.

Xiang-Gu · 2023-01-27T21:18:19Z

TFTR!

bors r+

craig · 2023-01-27T22:24:25Z

Build succeeded:

Bazel Essential CI (Cockroach)

ajwerner reviewed Jan 19, 2023

View reviewed changes

pkg/sql/backfill.go Outdated Show resolved Hide resolved

ajwerner reviewed Jan 19, 2023

View reviewed changes

pkg/sql/catalog/tabledesc/column.go Outdated Show resolved Hide resolved

ajwerner reviewed Jan 19, 2023

View reviewed changes

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go Outdated Show resolved Hide resolved

ajwerner reviewed Jan 19, 2023

View reviewed changes

pkg/sql/logictest/testdata/logic_test/alter_table Outdated Show resolved Hide resolved

ajwerner reviewed Jan 19, 2023

View reviewed changes

Xiang-Gu force-pushed the introduce-not-null-element branch 7 times, most recently from 9c628ff to c53bc23 Compare January 23, 2023 18:50

Xiang-Gu mentioned this pull request Jan 23, 2023

WIP: schemachanger: Move IsNullable from ColumnType to CheckConstraint #95137

Closed

Xiang-Gu force-pushed the introduce-not-null-element branch 6 times, most recently from 47ad22f to 5444e27 Compare January 24, 2023 01:28

Xiang-Gu marked this pull request as ready for review January 24, 2023 17:50

Xiang-Gu requested a review from a team January 24, 2023 17:50

Xiang-Gu requested a review from a team as a code owner January 24, 2023 17:50

Xiang-Gu requested a review from yuzefovich January 24, 2023 17:50

Xiang-Gu changed the title ~~WIP: schemachanger: Implement ALTER COLUMN ... SET NOT NULL~~ schemachanger: Implement ALTER COLUMN ... SET NOT NULL Jan 24, 2023

yuzefovich removed request for a team and yuzefovich January 24, 2023 18:05

ajwerner reviewed Jan 24, 2023

View reviewed changes

postamar reviewed Jan 25, 2023

View reviewed changes

Xiang-Gu force-pushed the introduce-not-null-element branch from 5444e27 to 643edf9 Compare January 26, 2023 00:22

Xiang-Gu commented Jan 26, 2023

View reviewed changes

Xiang-Gu force-pushed the introduce-not-null-element branch 6 times, most recently from 816d36c to 4bac39b Compare January 27, 2023 18:35

ajwerner reviewed Jan 27, 2023

View reviewed changes

pkg/sql/catalog/tabledesc/validate.go Outdated Show resolved Hide resolved

ajwerner approved these changes Jan 27, 2023

View reviewed changes

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_add_column.go Outdated Show resolved Hide resolved

Xiang-Gu force-pushed the introduce-not-null-element branch 2 times, most recently from 4093917 to d86e09e Compare January 27, 2023 20:25

Xiang-Gu added 4 commits January 27, 2023 16:17

scmutationexec: Use Go generics to replace a bunch of repetitive funcs

2f69d92

schemachanger: Introduce ColumnNotNull element

72ee36a

This commit introduce a new element protobuf `ColumnNotNull`, which will be used in later commit to re-implement how we handle NOT NULL constraint of a column.

schemachanger: Turn SET NOT NULL on by default and add tests

74dd4a1

Xiang-Gu force-pushed the introduce-not-null-element branch from d86e09e to 74dd4a1 Compare January 27, 2023 21:17

craig bot merged commit 8e82bf3 into cockroachdb:master Jan 27, 2023

rafiss mentioned this pull request Jan 22, 2025

sql/schemachanger: support ALTER COLUMN ... DROP NOT NULL in declarative schema changer #139602

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

schemachanger: Implement `ALTER COLUMN ... SET NOT NULL` #95556

schemachanger: Implement `ALTER COLUMN ... SET NOT NULL` #95556

Xiang-Gu commented Jan 19, 2023 •

edited

Loading

cockroach-teamcity commented Jan 19, 2023

ajwerner left a comment

Xiang-Gu commented Jan 24, 2023

Xiang-Gu commented Jan 24, 2023

ajwerner left a comment

postamar commented Jan 24, 2023

postamar left a comment

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

postamar Jan 25, 2023

Xiang-Gu left a comment

ajwerner left a comment

Xiang-Gu commented Jan 27, 2023

craig bot commented Jan 27, 2023

schemachanger: Implement ALTER COLUMN ... SET NOT NULL #95556

schemachanger: Implement ALTER COLUMN ... SET NOT NULL #95556

Conversation

Xiang-Gu commented Jan 19, 2023 • edited Loading

cockroach-teamcity commented Jan 19, 2023

ajwerner left a comment

Choose a reason for hiding this comment

Xiang-Gu commented Jan 24, 2023

Xiang-Gu commented Jan 24, 2023

ajwerner left a comment

Choose a reason for hiding this comment

postamar commented Jan 24, 2023

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xiang-Gu left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Xiang-Gu commented Jan 27, 2023

craig bot commented Jan 27, 2023

schemachanger: Implement `ALTER COLUMN ... SET NOT NULL` #95556

schemachanger: Implement `ALTER COLUMN ... SET NOT NULL` #95556

Xiang-Gu commented Jan 19, 2023 •

edited

Loading