Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: allow 'set default' cascading action when default is implicit null #38975

Closed
rolandcrosby opened this issue Jul 18, 2019 · 1 comment · Fixed by #39136
Closed

sql: allow 'set default' cascading action when default is implicit null #38975

rolandcrosby opened this issue Jul 18, 2019 · 1 comment · Fixed by #39136
Assignees
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@rolandcrosby
Copy link

While digging into #38933, @tyler314 ended up hitting an error involving this test:

statement ok
CREATE TABLE a (
  id STRING PRIMARY KEY
);
CREATE TABLE b (
  id STRING PRIMARY KEY
 ,a_id STRING DEFAULT NULL UNIQUE REFERENCES a ON DELETE SET DEFAULT
);
CREATE TABLE c (
  id STRING PRIMARY KEY
 ,b_a_id STRING REFERENCES b(a_id) ON UPDATE CASCADE
);

It turns out that removing the explicit DEFAULT NULL on b.a_id hits this error; we forbid set default cascading actions on any column without a default expression. However, elsewhere we treat null as the default default, so to speak; every column lacking an explicit 'default' expression should be treated as default null. And Postgres doesn't even distinguish between explicit default null and a lack of a default expression. As such, Postgres will happily execute the above CREATE TABLE statements even without the explicit DEFAULT NULL in b.a_id:

postgres=# select version();
                                                     version
-----------------------------------------------------------------------------------------------------------------
 PostgreSQL 11.4 on x86_64-apple-darwin18.6.0, compiled by Apple LLVM version 10.0.1 (clang-1001.0.46.4), 64-bit
(1 row)

postgres=# create table a (
  id varchar primary key
);
CREATE TABLE
postgres=# create table b (
  id varchar primary key
, a_id varchar unique references a on delete set default
);
CREATE TABLE
postgres=# create table c (
  id varchar primary key
, b_a_id varchar references b(a_id) on update cascade
);
CREATE TABLE
@rolandcrosby rolandcrosby added A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Jul 18, 2019
@tyler314
Copy link

What was the most confusing to me, is how do we hit this error in the case where the user explicitly sets DEFAULT NULL, but not when they do so implicitly. After my change, the Default Expression in the column is nil in both cases, but we only hit the error in the explicit case. Will continue to dig a little deeper.

@tyler314 tyler314 self-assigned this Jul 23, 2019
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Jul 27, 2019
Add support for implicitly setting the default value of a column
to null, for cascading tables for both ON DELETE and ON UPDATE.

The problem was in updateRows within cascader.go when the column ids
were being updated for a table. If a user did not explicitly specify
NULL as the default value, the default expression was dereferenced,
which was null. Instead of deferencing said value, and setting it to the
column id, we now just set the column id to nil if the default
expression is also nil. This is the same behavior for the explicit case.

Resolves: cockroachdb#38975

Release note (sql change): Now supports the implicit setting of NULL
values for foreign keys.
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Jul 31, 2019
Add support for implicitly setting the default value of a column
to null, for cascading tables for both ON DELETE and ON UPDATE.

The problem was in updateRows within cascader.go when the column ids
were being updated for a table. If a user did not explicitly specify
NULL as the default value, the default expression was dereferenced,
which was null. Instead of deferencing said value, and setting it to the
column id, we now just set the column id to nil if the default
expression is also nil. This is the same behavior for the explicit case.

Resolves: cockroachdb#38975

Release note (sql change): Columns without an explicit default value now
support foreign keys with the SET DEFAULT action.
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Jul 31, 2019
Add support for implicitly setting the default value of a column
to null, for cascading tables for both ON DELETE and ON UPDATE.

The problem was in updateRows within cascader.go when the column ids
were being updated for a table. If a user did not explicitly specify
NULL as the default value, the default expression was dereferenced,
which was null. Instead of deferencing said value, and setting it to the
column id, we now just set the column id to nil if the default
expression is also nil. This is the same behavior for the explicit case.

Resolves: cockroachdb#38975

Release note (sql change): Columns without an explicit default value now
support foreign keys with the SET DEFAULT action.
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Jul 31, 2019
Add support for implicitly setting the default value of a column
to null, for cascading tables for both ON DELETE and ON UPDATE.

The problem was in updateRows within cascader.go when the column ids
were being updated for a table. If a user did not explicitly specify
NULL as the default value, the default expression was dereferenced,
which was null. Instead of deferencing said value, and setting it to the
column id, we now just set the column id to nil if the default
expression is also nil. This is the same behavior for the explicit case.

Resolves: cockroachdb#38975

Release note (sql change): Columns without an explicit default value now
support foreign keys with the SET DEFAULT action.
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Aug 1, 2019
Add support for implicitly setting the default value of a column
to null, for cascading tables for both ON DELETE and ON UPDATE.

The problem was in updateRows within cascader.go when the column ids
were being updated for a table. If a user did not explicitly specify
NULL as the default value, the default expression was dereferenced,
which was null. Instead of deferencing said value, and setting it to the
column id, we now just set the column id to nil if the default
expression is also nil. This is the same behavior for the explicit case.

Resolves: cockroachdb#38975

Release note (sql change): Columns without an explicit default value now
support foreign keys with the SET DEFAULT action.
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Aug 6, 2019
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: cockroachdb#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.
tyler314 pushed a commit to tyler314/cockroach that referenced this issue Aug 6, 2019
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: cockroachdb#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.
craig bot pushed a commit that referenced this issue Aug 7, 2019
39136: sql: allow cascading action when default is set implicitly to null r=tyleroberts a=tyleroberts

Add support for implicitly setting the default value of a column
to null, for cascading tables for both ON DELETE and ON UPDATE.

The problem was in updateRows within cascader.go when the column ids
were being updated for a table. If a user did not explicitly specify
NULL as the default value, the default expression was dereferenced,
which was null. Instead of deferencing said value, and setting it to the
column id, we now just set the column id to nil if the default
expression is also nil. This is the same behavior for the explicit case.

Resolves: #38975

Release note (sql change): Now supports the implicit setting of NULL
values for foreign keys.

Co-authored-by: Tyler314 <tyler@cockroachlabs.com>
@craig craig bot closed this as completed in #39136 Aug 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants