Merge #150176

150176: sql: block DROP NOT NULL on generated identity columns r=fqazi a=fqazi

Previously, we incorrectly allowed users to drop not null on generated identity columns. The error we generated was vague and not instructive as to what was incorrect in this scenario. This patch, adds logic to block this operation in the declarative schema changer and improves the error message in the legacy one.

Fixes: #150021
Fixes: #148455

Release note: None

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

Author: craig[bot]

pkg/sql/alter_table.go

@@ -1058,7 +1058,11 @@ func applyColumnMutation(
 			return pgerror.Newf(pgcode.InvalidTableDefinition,
 				`column "%s" is in a primary index`, col.GetName())
 		}
-
+		// Ensure that we are not dropping not-null on a generated column.
+		if col.GetGeneratedAsIdentityType() != catpb.GeneratedAsIdentityType_NOT_IDENTITY_COLUMN {
+			return pgerror.Newf(pgcode.Syntax,
+				`column "%s" of relation "%s" is an identity column`, col.GetName(), tn.ObjectName)
+		}
 		// See if there's already a mutation to add/drop a not null constraint.
 		for i := range tableDesc.Mutations {
 			if constraint := tableDesc.Mutations[i].GetConstraint(); constraint != nil &&

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -5182,3 +5182,21 @@ t_128420  CREATE TABLE public.t_128420 (
           ) WITH (schema_locked = true);
 
 subtest end
+
+# Validate that dropping not-null is not allowed on generated identity columns
+# (#150021)
+subtest drop_not_null_identity
+
+statement ok
+CREATE TABLE drop_not_null_identity (a int, b text) WITH (schema_locked = false);
+
+statement ok
+ALTER TABLE drop_not_null_identity ALTER COLUMN a SET NOT NULL;
+
+statement ok
+ALTER TABLE drop_not_null_identity ALTER COLUMN a ADD GENERATED ALWAYS AS IDENTITY;
+
+statement error pgcode 42601 column \"a\" of relation \"drop_not_null_identity\" is an identity column
+ALTER TABLE drop_not_null_identity ALTER COLUMN a DROP NOT NULL
+
+subtest end

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

@@ -6,6 +6,7 @@
 package scbuildstmt
 
 import (
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scpb"
@@ -35,6 +36,13 @@ func alterTableDropNotNull(
 		panic(pgerror.Newf(pgcode.InvalidTableDefinition,
 			`column "%s" is in a primary index`, colName.Name))
 	}
+	// Ensure that we are not dropping not-null on a generated column.
+	colEl := mustRetrieveColumnElem(b, tbl.TableID, columnID)
+	if colEl.GeneratedAsIdentityType != catpb.GeneratedAsIdentityType_NOT_IDENTITY_COLUMN {
+		colName := mustRetrieveColumnName(b, tbl.TableID, columnID)
+		panic(pgerror.Newf(pgcode.Syntax,
+			`column "%s" of relation "%s" is an identity column`, colName.Name, tn.ObjectName))
+	}
 	columNotNull := b.QueryByID(tbl.TableID).FilterColumnNotNull().Filter(func(current scpb.Status, target scpb.TargetStatus, e *scpb.ColumnNotNull) bool {
 		return e.ColumnID == columnID
 	}).MustGetZeroOrOneElement()