From 5eedebc529adedc33b93ca5146aed82479a9c40e Mon Sep 17 00:00:00 2001 From: Annie Pompa Date: Fri, 19 Jul 2024 16:16:46 -0400 Subject: [PATCH] sql: block set default on computed col This patch blocks setting/dropping a default (even a null default) on a computed column in the LSC and DSC. Fixes: #127522 Release note (bug fix): Setting or dropping a default value on a computed column is now blocked -- even for null defaults. Previously, setting or dropping a default value on a computed column was a no-op. --- pkg/sql/alter_table.go | 19 ++++++++++++ pkg/sql/catalog/tabledesc/validate.go | 2 +- .../logictest/testdata/logic_test/computed | 25 +++++++++++++++- .../alter_table_alter_column_set_default.go | 29 +++++++++++++++++-- 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/pkg/sql/alter_table.go b/pkg/sql/alter_table.go index 621052038704..75304b16790e 100644 --- a/pkg/sql/alter_table.go +++ b/pkg/sql/alter_table.go @@ -943,6 +943,25 @@ func applyColumnMutation( return AlterColumnType(ctx, tableDesc, col, t, params, cmds, tn) case *tree.AlterTableSetDefault: + // If our column is computed, block mixing defaults in entirely. + // This check exists here instead of later on during validation because + // adding a null default to a computed column should also be blocked, but + // is undetectable later on since SET DEFAULT NUL means a nil default + // expression. + if col.IsComputed() { + // Block dropping a computed column "default" as well. + if t.Default == nil { + return pgerror.Newf( + pgcode.Syntax, + "column %q of relation %q is a computed column", + col.GetName(), + tn.ObjectName) + } + return pgerror.Newf( + pgcode.Syntax, + "computed column %q cannot also have a DEFAULT expression", + col.GetName()) + } if err := updateNonComputedColExpr( params, tableDesc, diff --git a/pkg/sql/catalog/tabledesc/validate.go b/pkg/sql/catalog/tabledesc/validate.go index 22809d67f5ac..4cc36d4f725e 100644 --- a/pkg/sql/catalog/tabledesc/validate.go +++ b/pkg/sql/catalog/tabledesc/validate.go @@ -1143,7 +1143,7 @@ func (desc *wrapper) validateColumns() error { if column.IsComputed() { if column.HasDefault() { - return pgerror.Newf(pgcode.InvalidTableDefinition, + return pgerror.Newf(pgcode.Syntax, "computed column %q cannot also have a DEFAULT expression", column.GetName(), ) diff --git a/pkg/sql/logictest/testdata/logic_test/computed b/pkg/sql/logictest/testdata/logic_test/computed index 1bd896f4d148..484e20ccb633 100644 --- a/pkg/sql/logictest/testdata/logic_test/computed +++ b/pkg/sql/logictest/testdata/logic_test/computed @@ -1137,7 +1137,7 @@ SELECT * FROM t69327 f f # Regression test for #72881. Computed columns can't have a DEFAULT expr. -statement error pgcode 42P16 computed column "v" cannot also have a DEFAULT expression +statement error pgcode 42601 computed column "v" cannot also have a DEFAULT expression ALTER TABLE t69327 ALTER COLUMN v SET DEFAULT 'foo' # Regression test for #69665.Computed columns should be evaluated after @@ -1184,3 +1184,26 @@ FROM t88128 ---- b1 expected_b1 b2 expected_b2 true true true true + +# Regression test for #127522 where we do not properly block adding a default +# value to a computed column. +subtest computed_column_with_default + +statement ok +CREATE TABLE foooooo ( + id INT PRIMARY KEY, + x INT NOT NULL, + y INT NOT NULL, + gen INT AS (x + y) STORED +); + +statement error pgcode 42601 computed column "gen" cannot also have a DEFAULT expression +ALTER TABLE foooooo ALTER COLUMN gen SET DEFAULT 1; + +statement error pgcode 42601 computed column "gen" cannot also have a DEFAULT expression +ALTER TABLE foooooo ALTER COLUMN gen SET DEFAULT NULL; + +statement error pgcode 42601 column "gen" of relation "foooooo" is a computed column +ALTER TABLE foooooo ALTER COLUMN gen DROP DEFAULT; + +subtest end diff --git a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_default.go b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_default.go index 9dc9b0e7af08..c1ee42e2bef3 100644 --- a/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_default.go +++ b/pkg/sql/schemachanger/scbuild/internal/scbuildstmt/alter_table_alter_column_set_default.go @@ -32,6 +32,13 @@ func alterTableSetDefault( colID := getColumnIDFromColumnName(b, tbl.TableID, t.Column, true /* required */) col := mustRetrieveColumnElem(b, tbl.TableID, colID) oldDefaultExpr := retrieveColumnDefaultExpressionElem(b, tbl.TableID, colID) + colType := mustRetrieveColumnTypeElem(b, tbl.TableID, colID) + + // Block alters on system columns. + panicIfSystemColumn(col, t.Column.String()) + + // Block disallowed operations on computed columns. + panicIfComputedColumn(tn.ObjectName, colType, t.Column.String(), t.Default) // For DROP DEFAULT. if t.Default == nil { @@ -41,9 +48,6 @@ func alterTableSetDefault( return } - // Block alters on system columns. - panicIfSystemColumn(col, t.Column.String()) - // If our target column already has a default expression, we want to drop it first. if oldDefaultExpr != nil { b.Drop(oldDefaultExpr) @@ -142,3 +146,22 @@ func sanitizeColumnExpression( s := tree.Serialize(typedExpr) return typedExpr, s, nil } + +// panicIfComputedColumn blocks disallowed operations on computed columns. +func panicIfComputedColumn(tn tree.Name, col *scpb.ColumnType, colName string, def tree.Expr) { + // Block setting a column default if the column is computed. + if col.ComputeExpr != nil { + // Block dropping a computed col "default" as well. + if def == nil { + panic(pgerror.Newf( + pgcode.Syntax, + "column %q of relation %q is a computed column", + colName, + tn)) + } + panic(pgerror.Newf( + pgcode.Syntax, + "computed column %q cannot also have a DEFAULT expression", + colName)) + } +}