Merge #148038

148038: sql/schemachanger: add support for DROP NOT NULL r=fqazi a=fqazi

Previously, ALTER TABLE ... ALTER COLUMN ... DROP NOT NULL was only supported in the legacy schema changer. This patch adds support for dropping not null constraints in the declarative schema changer.

Fixes: #139602
Release note: None

Co-authored-by: Faizan Qazi <[email protected]>

Author: craig[bot]

pkg/ccl/logictestccl/testdata/logic_test/redact_descriptor

@@ -47,6 +47,7 @@ CREATE VIEW redacted_descriptors AS
 # descriptor. We will skip the config that uses legacy schema changer.
 skipif config local-schema-locked
 skipif config local-legacy-schema-changer
+skipif config local-mixed-25.2
 query T
 SELECT descriptor from redacted_descriptors where id = 'collate_partition'::REGCLASS;
 ----

pkg/ccl/schemachangerccl/sctestbackupccl/backup_base_generated_test.go

@@ -158,9 +158,6 @@ var enabledDjangoTests = []string{
 
 // Maintain that this list is alphabetized.
 var djangoBlocklist = blocklist{
-	// Schema change used by this test:
-	// ALTER TABLE schema_author ALTER COLUMN name SET DATA TYPE STRING, ALTER COLUMN name DROP NOT NULL
-	`schema.tests.SchemaTests.test_alter`:                              "ALTER COLUMN ... DROP NOT NULL is not supported in declarative schema changer (prevents from working with schema_locked)",
 	`schema.tests.SchemaTests.test_alter_text_field_to_date_field`:     "alter type requires USING",
 	`schema.tests.SchemaTests.test_alter_text_field_to_datetime_field`: "alter type requires USING",
 	`schema.tests.SchemaTests.test_alter_text_field_to_time_field`:     "alter type requires USING",

pkg/cmd/roachtest/tests/django_blocklist.go

@@ -7,6 +7,7 @@ go_library(
         "alter_table.go",
         "alter_table_add_column.go",
         "alter_table_add_constraint.go",
+        "alter_table_alter_column_drop_not_null.go",
         "alter_table_alter_column_set_default.go",
         "alter_table_alter_column_set_not_null.go",
         "alter_table_alter_column_type.go",

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/BUILD.bazel

@@ -40,6 +40,7 @@ var supportedAlterTableStatements = map[reflect.Type]supportedAlterTableCommand{
 	reflect.TypeOf((*tree.AlterTableSetDefault)(nil)):         {fn: alterTableSetDefault, on: true, checks: nil},
 	reflect.TypeOf((*tree.AlterTableAlterColumnType)(nil)):    {fn: alterTableAlterColumnType, on: true, checks: nil},
 	reflect.TypeOf((*tree.AlterTableSetRLSMode)(nil)):         {fn: alterTableSetRLSMode, on: true, checks: isV252Active},
+	reflect.TypeOf((*tree.AlterTableDropNotNull)(nil)):        {fn: alterTableDropNotNull, on: true, checks: isV253Active},
 }
 
 func init() {

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

@@ -0,0 +1,45 @@
+// Copyright 2025 The Cockroach Authors.
+//
+// Use of this software is governed by the CockroachDB Software License
+// included in the /LICENSE file.
+
+package scbuildstmt
+
+import (
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
+)
+
+func alterTableDropNotNull(
+	b BuildCtx,
+	tn *tree.TableName,
+	tbl *scpb.Table,
+	stmt tree.Statement,
+	t *tree.AlterTableDropNotNull,
+) {
+	alterColumnPreChecks(b, tn, tbl, t.Column)
+	columnID := getColumnIDFromColumnName(b, tbl.TableID, t.Column, true /*required */)
+	// Block alters on system columns.
+	panicIfSystemColumn(mustRetrieveColumnElem(b, tbl.TableID, columnID), t.Column.String())
+	// Ensure that this column is not in the primary indexes key.
+	primaryIdx := getLatestPrimaryIndex(b, tbl.TableID)
+	idxColumns := mustRetrieveIndexColumnElements(b, tbl.TableID, primaryIdx.IndexID)
+	for _, idxCol := range idxColumns {
+		if idxCol.ColumnID != columnID ||
+			idxCol.Kind != scpb.IndexColumn_KEY {
+			continue
+		}
+		colName := mustRetrieveColumnName(b, tbl.TableID, columnID)
+		panic(pgerror.Newf(pgcode.InvalidTableDefinition,
+			`column "%s" is in a primary index`, colName.Name))
+	}
+	columNotNull := b.QueryByID(tbl.TableID).FilterColumnNotNull().Filter(func(current scpb.Status, target scpb.TargetStatus, e *scpb.ColumnNotNull) bool {
+		return e.ColumnID == columnID
+	}).MustGetZeroOrOneElement()
+	if columNotNull == nil {
+		return
+	}
+	b.Drop(columNotNull)
+}

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

@@ -217,3 +217,7 @@ var isV251Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMo
 var isV252Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMode, activeVersion clusterversion.ClusterVersion) bool {
 	return activeVersion.IsActive(clusterversion.V25_2)
 }
+
+var isV253Active = func(_ tree.NodeFormatter, _ sessiondatapb.NewSchemaChangerMode, activeVersion clusterversion.ClusterVersion) bool {
+	return activeVersion.IsActive(clusterversion.V25_3)
+}

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

@@ -12,10 +12,6 @@ CREATE TABLE defaultdb.foo (
 );
 ----
 
-unimplemented
-ALTER TABLE defaultdb.foo ALTER COLUMN i DROP NOT NULL
-----
-
 unimplemented
 ALTER TABLE defaultdb.foo ALTER COLUMN i DROP STORED
 ----

pkg/sql/schemachanger/scbuild/testdata/unimplemented_alter_table

@@ -0,0 +1,28 @@
+setup
+CREATE TABLE t (i INT PRIMARY KEY, j INT NOT NULL);
+----
+
+# Ensure that inserts with non-null values will succeed.
+stage-exec phase=PostCommitNonRevertiblePhase stage=:
+INSERT INTO t VALUES ($stageKey, $stageKey);
+DELETE FROM t WHERE j = $stageKey;
+INSERT INTO t VALUES ($stageKey, $stageKey);
+UPDATE t SET j = j + 1;
+----
+
+# Validate the not null constraint is enforced initially
+stage-exec phase=PostCommitNonRevertiblePhase stage=1
+INSERT INTO t(i) VALUES(-1)
+----
+pq: failed to satisfy CHECK constraint .*
+
+
+# One row is expected to be added after each stage.
+stage-query phase=PostCommitNonRevertiblePhase stage=:
+SELECT count(*)=$successfulStageCount FROM t;
+----
+true
+
+test
+ALTER TABLE t ALTER COLUMN j DROP NOT NULL
+----