From 5f124651993efadcfd31a71340d175443d8ec079 Mon Sep 17 00:00:00 2001 From: Tyler314 Date: Fri, 26 Jul 2019 20:34:34 -0700 Subject: [PATCH] sql: allow cascading action when default is set implicitly to null Add support for implicitly setting the default value of a column to null, for cascading tables for both ON DELETE and ON UPDATE. Previously, if a user set a reference to a column and added a SET DEFAULT for an ON DELETE/UPDATE cascading action, without an explicit default expression, CRDB would forbid it. Postgres does not distinguish between SET NULL and the absence of a default expression. Thus, the change is to also have CRDB not distinguish between the two cases. In the situation where a user creates a table with an explicit default null via SET NULL, CRDB does not allow the user to create the table at all. This deviates from how postgres handles it. Postgres allows the user to create the table but throws an error if a cascading action occurs that uses the default value. The new changes now prevent the user from creating a table when they constrain the columns to be non-nullable, and implicitly set the default to NULL. This is done to match the behavior for the explicit case of setting the default to NULL. Resolves: #38975 Release note (sql change): Columns without an explicit default value now support foreign keys with the SET DEFAULT action, in the same way as the SET NULL and SET DEFAULT NULL cases. --- pkg/sql/create_table.go | 11 +- pkg/sql/logictest/testdata/logic_test/fk | 105 +++++++++++++++----- pkg/sql/logictest/testdata/logic_test/table | 2 +- pkg/sql/row/cascader.go | 5 + pkg/sql/sqlbase/structured.go | 30 ++++-- pkg/sql/sqlbase/structured_test.go | 37 ++++++- pkg/sql/sqlbase/table.go | 13 ++- 7 files changed, 156 insertions(+), 47 deletions(-) diff --git a/pkg/sql/create_table.go b/pkg/sql/create_table.go index 364bb6d5f6a1..ebe716c43b57 100644 --- a/pkg/sql/create_table.go +++ b/pkg/sql/create_table.go @@ -599,14 +599,17 @@ func ResolveFK( } } - // Don't add a SET DEFAULT action on an index that has any column that does - // not have a DEFAULT expression. + // Don't add a SET DEFAULT action on an index that has any column that has + // a DEFAULT expression of NULL and a NOT NULL constraint. if d.Actions.Delete == tree.SetDefault || d.Actions.Update == tree.SetDefault { for _, sourceColumn := range srcCols { - if sourceColumn.DefaultExpr == nil { + // Having a default expression of NULL, and a constraint of NOT NULL is a + // contradiction and should never be allowed. + if sourceColumn.DefaultExpr == nil && !sourceColumn.Nullable { col := qualifyFKColErrorWithDB(ctx, txn, tbl.TableDesc(), sourceColumn.Name) return pgerror.Newf(pgcode.InvalidForeignKey, - "cannot add a SET DEFAULT cascading action on column %q which has no DEFAULT expression", col, + "cannot add a SET DEFAULT cascading action on column %q which has a "+ + "NOT NULL constraint and a NULL default expression", col, ) } } diff --git a/pkg/sql/logictest/testdata/logic_test/fk b/pkg/sql/logictest/testdata/logic_test/fk index a4d39cfac7a8..e7110c35eb3a 100644 --- a/pkg/sql/logictest/testdata/logic_test/fk +++ b/pkg/sql/logictest/testdata/logic_test/fk @@ -1201,25 +1201,26 @@ CREATE TABLE a ( ); # Create a table with no DEFAULT expressions column and a SET DEFAULT action. -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.delete_no_default" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement ok +CREATE TABLE delete_no_default_table ( id INT PRIMARY KEY ,delete_no_default INT REFERENCES a ON DELETE SET DEFAULT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.update_no_default" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.update_no_default_table.update_no_default" which has a NOT NULL constraint and a NULL default expression +CREATE TABLE update_no_default_table ( id INT PRIMARY KEY ,update_no_default INT NOT NULL REFERENCES a ON UPDATE SET DEFAULT ); # Create a table where the primary key has a SET DEFAULT action. -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has no DEFAULT expression -CREATE TABLE primary_key_table ( +# Primary keys are not allowed to be NULL +statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table_set_default.id" which has a NOT NULL constraint and a NULL default expression +CREATE TABLE primary_key_table_set_default ( id INT PRIMARY KEY REFERENCES a ON DELETE SET DEFAULT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has no DEFAULT expression +statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has a NOT NULL constraint and a NULL default expression CREATE TABLE primary_key_table ( id INT PRIMARY KEY REFERENCES a ON UPDATE SET DEFAULT ); @@ -1232,12 +1233,12 @@ CREATE TABLE no_default_table ( ,update_no_default INT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.delete_no_default" which has no DEFAULT expression +statement ok ALTER TABLE no_default_table ADD CONSTRAINT no_default_delete_set_default FOREIGN KEY (delete_no_default) REFERENCES a (id) ON DELETE SET DEFAULT; -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.update_no_default" which has no DEFAULT expression +statement ok ALTER TABLE no_default_table ADD CONSTRAINT no_default_update_set_default FOREIGN KEY (update_no_default) REFERENCES a (id) ON UPDATE SET DEFAULT; @@ -1253,19 +1254,20 @@ CREATE TABLE primary_key_table ( id INT PRIMARY KEY ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has no DEFAULT expression +# id is a primary key and thus cannot be NULL +statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has a NOT NULL constraint and a NULL default expression ALTER TABLE primary_key_table ADD CONSTRAINT no_default_delete_set_default FOREIGN KEY (id) REFERENCES a (id) ON DELETE SET DEFAULT; -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has no DEFAULT expression +statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has a NOT NULL constraint and a NULL default expression ALTER TABLE primary_key_table ADD CONSTRAINT no_default_update_set_default FOREIGN KEY (id) REFERENCES a (id) ON UPDATE SET DEFAULT; # Clean up the tables used so far. statement ok -DROP TABLE primary_key_table, a; +DROP TABLE primary_key_table, delete_no_default_table, a; # Now test composite foreign keys statement ok @@ -1276,7 +1278,7 @@ CREATE TABLE a ( ); # Create a table with a column without a DEFAULT expression and a SET DEFAULT action. -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.ref1" which has no DEFAULT expression +statement ok CREATE TABLE no_default_table ( id INT PRIMARY KEY ,ref1 INT @@ -1285,8 +1287,32 @@ CREATE TABLE no_default_table ( ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON DELETE SET DEFAULT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.ref1" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement ok +INSERT INTO a VALUES (1, 2) + +statement ok +INSERT INTO a VALUES (3, 4) + +statement ok +INSERT INTO no_default_table VALUES (6, 2, 1) + +query III colnames +SELECT * FROM no_default_table +---- +id ref1 ref2 +6 2 1 + +statement ok +DELETE FROM a WHERE id1=1 + +query III colnames +SELECT * FROM no_default_table +---- +id ref1 ref2 +6 NULL NULL + +statement ok +CREATE TABLE no_default_table_on_update ( id INT PRIMARY KEY ,ref1 INT ,ref2 INT @@ -1294,8 +1320,26 @@ CREATE TABLE no_default_table ( ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON UPDATE SET DEFAULT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.ref1" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement ok +INSERT INTO no_default_table_on_update VALUES (0, 4, 3) + +query III colnames +SELECT * FROM no_default_table_on_update +---- +id ref1 ref2 +0 4 3 + +statement ok +UPDATE a SET id1=33, id2=44 WHERE id1=3; + +query III colnames +SELECT * FROM no_default_table_on_update +---- +id ref1 ref2 +0 NULL NULL + +statement ok +CREATE TABLE no_default_table_ref2_default_on_delete ( id INT PRIMARY KEY ,ref1 INT ,ref2 INT DEFAULT 1 @@ -1303,9 +1347,8 @@ CREATE TABLE no_default_table ( ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON DELETE SET DEFAULT ); - -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.ref1" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement ok +CREATE TABLE no_default_table_ref2_default_on_update ( id INT PRIMARY KEY ,ref1 INT ,ref2 INT DEFAULT 1 @@ -1313,8 +1356,8 @@ CREATE TABLE no_default_table ( ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON UPDATE SET DEFAULT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.ref2" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement ok +CREATE TABLE no_default_table_ref1_default_on_delete ( id INT PRIMARY KEY ,ref1 INT DEFAULT 1 ,ref2 INT @@ -1322,8 +1365,8 @@ CREATE TABLE no_default_table ( ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON DELETE SET DEFAULT ); -statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.no_default_table.ref2" which has no DEFAULT expression -CREATE TABLE no_default_table ( +statement ok +CREATE TABLE no_default_table_ref1_default_on_update ( id INT PRIMARY KEY ,ref1 INT DEFAULT 1 ,ref2 INT @@ -1331,9 +1374,21 @@ CREATE TABLE no_default_table ( ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON UPDATE SET DEFAULT ); +# Create a table with a NOT NULL column and a SET NULL action. +statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.not_null_table.ref1" which has a NOT NULL constraint and a NULL default expression +CREATE TABLE not_null_table ( + id INT PRIMARY KEY + ,ref1 INT NOT NULL + ,ref2 INT NOT NULL + ,INDEX (ref1, ref2) + ,FOREIGN KEY (ref1, ref2) REFERENCES a (id2, id1) ON DELETE SET DEFAULT +); + # Clean up after the test. statement ok -DROP TABLE a; +DROP TABLE a, no_default_table, no_default_table_on_update, no_default_table_ref2_default_on_delete, +no_default_table_ref2_default_on_update, no_default_table_ref1_default_on_delete, +no_default_table_ref1_default_on_update # Test that no column can have more than 1 FK constraint subtest column_uniqueness diff --git a/pkg/sql/logictest/testdata/logic_test/table b/pkg/sql/logictest/testdata/logic_test/table index 3c2de995b852..6004e9a80a21 100644 --- a/pkg/sql/logictest/testdata/logic_test/table +++ b/pkg/sql/logictest/testdata/logic_test/table @@ -461,7 +461,7 @@ query TT SHOW CREATE TABLE test.null_default ---- test.public.null_default CREATE TABLE null_default ( - ts TIMESTAMP NULL DEFAULT NULL, + ts TIMESTAMP NULL, FAMILY "primary" (ts, rowid) ) diff --git a/pkg/sql/row/cascader.go b/pkg/sql/row/cascader.go index 55ddcf09f788..47f6ed0a5516 100644 --- a/pkg/sql/row/cascader.go +++ b/pkg/sql/row/cascader.go @@ -730,6 +730,11 @@ func (c *cascader) updateRows( if err != nil { return nil, nil, nil, 0, err } + // If the default expression is nil, treat it as a SET NULL case. + if column.HasNoDefault() { + referencingIndexValuesByColIDs[columnID] = tree.DNull + continue + } parsedExpr, err := parser.ParseExpr(*column.DefaultExpr) if err != nil { return nil, nil, nil, 0, err diff --git a/pkg/sql/sqlbase/structured.go b/pkg/sql/sqlbase/structured.go index 3e528f2e3372..9caeb949fdfb 100644 --- a/pkg/sql/sqlbase/structured.go +++ b/pkg/sql/sqlbase/structured.go @@ -2867,17 +2867,7 @@ func (desc *ImmutableTableDescriptor) MakeFirstMutationPublic( // ColumnNeedsBackfill returns true if adding the given column requires a // backfill (dropping a column always requires a backfill). func ColumnNeedsBackfill(desc *ColumnDescriptor) bool { - // Consider the case where the user explicitly states the default value of a - // new column to be NULL. desc.DefaultExpr is not nil, but the string "NULL" - if desc.DefaultExpr != nil { - if defaultExpr, err := parser.ParseExpr(*desc.DefaultExpr); err != nil { - panic(errors.NewAssertionErrorWithWrappedErrf(err, - "failed to parse default expression %s", *desc.DefaultExpr)) - } else if defaultExpr == tree.DNull { - return false - } - } - return desc.DefaultExpr != nil || !desc.Nullable || desc.IsComputed() + return !desc.HasNoDefault() || desc.HasNullDefault() || !desc.Nullable || desc.IsComputed() } // HasColumnBackfillMutation returns whether the table has any queued column @@ -3247,6 +3237,24 @@ func (desc *ColumnDescriptor) IsNullable() bool { return desc.Nullable } +// Checks that the column descriptor has no default. +func (desc *ColumnDescriptor) HasNoDefault() bool { + return desc.DefaultExpr == nil +} + +// Checks that the column descriptor has a default of NULL. +func (desc *ColumnDescriptor) HasNullDefault() bool { + if desc.HasNoDefault() { + return false + } + defaultExpr, err := parser.ParseExpr(*desc.DefaultExpr) + if err != nil { + panic(errors.NewAssertionErrorWithWrappedErrf(err, + "failed to parse default expression %s", *desc.DefaultExpr)) + } + return defaultExpr == tree.DNull +} + // ColID is part of the cat.Column interface. func (desc *ColumnDescriptor) ColID() cat.StableID { return cat.StableID(desc.ID) diff --git a/pkg/sql/sqlbase/structured_test.go b/pkg/sql/sqlbase/structured_test.go index 9411fed27b1f..e5544221bb17 100644 --- a/pkg/sql/sqlbase/structured_test.go +++ b/pkg/sql/sqlbase/structured_test.go @@ -1310,7 +1310,7 @@ func TestKeysPerRow(t *testing.T) { } func TestColumnNeedsBackfill(t *testing.T) { - // Define variable strings here such that we can pass their address below + // Define variable strings here such that we can pass their address below. null := "NULL" four := "4:::INT8" // Create Column Descriptors that reflect the definition of a column with a @@ -1757,3 +1757,38 @@ func TestUpgradeDowngradeFKRepr(t *testing.T) { } } } + +func TestDefaultExprNil(t *testing.T) { + s, conn, _ := serverutils.StartServer(t, base.TestServerArgs{}) + defer s.Stopper().Stop(context.TODO()) + if _, err := conn.Exec(`CREATE DATABASE t`); err != nil { + t.Fatalf("%+v", err) + } + t.Run(fmt.Sprintf("%s - %d", "(a INT PRIMARY KEY)", 1), func(t *testing.T) { + sqlDB := sqlutils.MakeSQLRunner(conn) + // Execute SQL commands with both implicit and explicit setting of the + // default expression. + sqlDB.Exec(t, `CREATE TABLE t (a INT PRIMARY KEY)`) + sqlDB.Exec(t, `INSERT INTO t (a) VALUES (1), (2)`) + sqlDB.Exec(t, `ALTER TABLE t ADD COLUMN b INT NULL`) + sqlDB.Exec(t, `INSERT INTO t (a) VALUES (3)`) + sqlDB.Exec(t, `ALTER TABLE t ADD COLUMN c INT DEFAULT NULL`) + + var descBytes []byte + // Grab the most recently created descriptor. + row := sqlDB.QueryRow(t, + `SELECT descriptor FROM system.descriptor ORDER BY id DESC LIMIT 1`) + row.Scan(&descBytes) + var desc Descriptor + if err := protoutil.Unmarshal(descBytes, &desc); err != nil { + t.Fatalf("%+v", err) + } + // Test and verify that the default expressions of the column descriptors + // are all nil. + for _, col := range desc.GetTable().Columns { + if col.DefaultExpr != nil { + t.Errorf("expected Column Default Expression to be 'nil', got %s instead.", *col.DefaultExpr) + } + } + }) +} diff --git a/pkg/sql/sqlbase/table.go b/pkg/sql/sqlbase/table.go index 8964e529d0a8..b9ec76ee98fc 100644 --- a/pkg/sql/sqlbase/table.go +++ b/pkg/sql/sqlbase/table.go @@ -168,12 +168,15 @@ func MakeColumnDefDescs( ); err != nil { return nil, nil, nil, err } - // We keep the type checked expression so that the type annotation - // gets properly stored. - d.DefaultExpr.Expr = typedExpr - s := tree.Serialize(d.DefaultExpr.Expr) - col.DefaultExpr = &s + // Keep the type checked expression so that the type annotation gets + // properly stored, only if the default expression is not NULL. + // Otherwise we want to keep the default expression nil. + if typedExpr != tree.DNull { + d.DefaultExpr.Expr = typedExpr + s := tree.Serialize(d.DefaultExpr.Expr) + col.DefaultExpr = &s + } } if d.IsComputed() {