sql: allow cascading action when default is set implicitly to null

[tyler314]

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.

[cockroach-teamcity]

This change is 

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @tyleroberts)

---

pkg/sql/logictest/testdata/logic_test/fk, line 1289 at r1 (raw file):

INSERT INTO no_default_table VALUES (6, 2, 1)

---

pkg/sql/row/cascader.go, line 733 at r1 (raw file):

				return nil, nil, nil, 0, err
			}
			// If the default expression is nil, treat it as a SET NULL case

nit: please make comments fully grammatical English sentences. In this case, that means putting a period at the end of the sentence. (This kind of thing is also mentioned in our style guide.)

---

pkg/sql/sqlbase/structured.go, line 2648 at r1 (raw file):

	// 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 {

I don't think that you can remove this case, because old descriptors could still have NULL inside their DefaultExpr field.

---

pkg/sql/sqlbase/structured_test.go, line 1312 at r1 (raw file):

func TestColumnNeedsBackfill(t *testing.T) {
	// Define variable strings here such that we can pass their address below

Hm, why delete this test? The new code does not seem to have any non-logic tests that verify the functionality. Rather then delete these, I'd prefer to see them updated to contain the behavior you expect.

---

pkg/sql/sqlbase/table.go, line 175 at r1 (raw file):

		// fact "NULL". It would be "NULL" instead of nil is the client explicitly
		// set the default to NULL.
		if d.DefaultExpr.Expr == tree.DNull {

Shouldn't you be checking typedExpr here instead of d.DefaultExpr.Expr?

---

pkg/sql/sqlbase/table.go, line 176 at r1 (raw file):

		// set the default to NULL.
		if d.DefaultExpr.Expr == tree.DNull {
			d.DefaultExpr.Expr = nil

I don't like the idea of mutating the input to this function - I see we do it below to set it to the result of that sanitizeVarFreeExpr method but I don't think it's necessary here. In general, mutating AST nodes is a no-no, as it can cause race conditions and generally make ASTs non-cachable. I think just doing nothing in this block would be more correct - in other words, only set col.DefaultExpr is `d.DefaultExpr.Expr is non-null. Does that work, or does it cause other issues?

---

pkg/sql/sqlbase/table.go, line 177 at r1 (raw file):

		if d.DefaultExpr.Expr == tree.DNull {
			d.DefaultExpr.Expr = nil
			col.DefaultExpr = nil

nit: I don't think it's necessary to set DefaultExpr on col here, since you just initialized col above, and all pointer fields on structs default to nil.

[thoszhang]

This release note is somewhat vague. Can it be something like "Columns with no explicit default value now support foreign keys with SET DEFAULT actions, using an implicit NULL default value"?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @tyleroberts)

---

pkg/sql/logictest/testdata/logic_test/fk, line 1222 at r1 (raw file):

);

statement error pq: cannot add a SET DEFAULT cascading action on column "test.public.primary_key_table.id" which has no DEFAULT expression

Is there a reason to not modify this test like the others, instead of removing it?

---

pkg/sql/logictest/testdata/logic_test/fk, line 1261 at r1 (raw file):

  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

Same question as above

---

pkg/sql/logictest/testdata/logic_test/fk, line 1279 at r1 (raw file):

statement ok
INSERT INTO a VALUES (1, 2)

Do we have any tests for the non-composite FK case, or where the referencing column is the primary key? It might be good to take some of the logic tests for SET NULL and adapt them for SET DEFAULT with a null default.

---

pkg/sql/sqlbase/structured.go, line 2648 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I don't think that you can remove this case, because old descriptors could still have NULL inside their DefaultExpr field.

Only new columns get backfilled, and this PR also starts setting the default to nil in column descs, so I think removing this case will be fine.

I suppose someone could have started a schema change to add a column, and then upgraded to this version before the schema change was complete, but I'm fine with that edge case.

[BramGruneir]

I think you included way too many commits. Reviewable is barfing at me.

Please rebase this against master.

[BramGruneir]

So one thing to consider is our basic axiom of "What does postgres do?" What does it do in these cases and how do we differ. Is that difference better, worse or neutral? If it's not better, we should instead default (pun intended) to what postgres does.

[thoszhang]

It is not obvious to me in what scenario a user would want to add a constraint on NOT NULL, and also have the default values of those same columns be NULL.

To be clear, we do allow this, and people do create NOT NULL columns with no default value; it's just a question of which FK delete/update actions we allow.

@BramGruneir I think you added those checks originally for SET NULL/SET DEFAULT on nullable columns - why did we decide to do something different from postgres? (I don't really have an opinion, and I think it's fine either way.)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @lucy-zhang)

[BramGruneir]

I think you added those checks originally for SET NULL/SET DEFAULT on nullable columns - why did we decide to do something different from postgres? (I don't really have an opinion, and I think it's fine either way.)

Honestly, I don't remember why. I think it was to try to be nice to our users. But there clearly are many cases in which this can be violated and this protection isn't really needed. At this point, I think it would be best to go the postgres route. BUT if we do, then the default default (ha!) should show up in a show create table statement instead of being implicit.

[tyler314]

I agree, but I do think this should be a separate PR. Because this change mimics the behavior of a pre-existing CRDB feature, I think it should still be merged. Thoughts?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @lucy-zhang)

[knz]

I have looked at the PR and came to the realization that a conceptual mistake was made.

As a result of a previous discussion some of us may have come to the understanding that "no DEFAULT expression" is equivalent to "DEFAULT NULL". I think the PR discussion has amply proven that this equivalence only holds if the column is nullable.

If the column is non-nullable, then "no DEFAULT expression" is certainly not equivalent to "DEFAULT NULL". In that case, "no DEFAULT" means "it's not possible to insert data OR update FKs unless the value is provided explicitly".

Once we collectively acknowledge this distinction, a lot of the discussion evaporates. See my comments below.

Reviewed 3 of 7 files at r1, 4 of 13 files at r2, 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @tyleroberts)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

			// 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 {

I think it's confusing to see this condition here. The check on the two fields DefaultExpr and Nullable should be done earlier (and rejected there), and you should be able to let the control flow arrive here and assume that the condition does not occur.

---

pkg/sql/create_table.go, line 604 at r4 (raw file):

			if sourceColumn.DefaultExpr == nil && !sourceColumn.Nullable {
				col := qualifyFKColErrorWithDB(ctx, txn, tbl.TableDesc(), sourceColumn.Name)
				return pgerror.Newf(pgcode.InvalidForeignKey,

Meanwhile, I think there is a missing check: we should reject SET DEFAULT unless either there is a DEFAULT expression, or there is not DEFAULT but the column is nullable.

(acknowledging that DEfaultExpr == nil means "no default" instead of "DEFAULT NULL")

---

pkg/sql/row/cascader.go, line 734 at r4 (raw file):

			}
			// If the default expression is nil, treat it as a SET NULL case.
			if column.DefaultExpr == nil {

Out of curiosity, where's the integrity check that the cascading action is only allowed if the column is nullable? Maybe reference that in the comment.

(Or better yet: properly distinguish DefaultExpr == nil from DefaultExpr == tree.DNull, as commented above).

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, and @tyleroberts)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, knz (kena) wrote…

I think it's confusing to see this condition here. The check on the two fields DefaultExpr and Nullable should be done earlier (and rejected there), and you should be able to let the control flow arrive here and assume that the condition does not occur.

I'm confused by this - where do you think the rejection should occur? Should we reject this when we create/add the column (and disallow, e.g.,create table t (a int not null default null)?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @tyleroberts)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I'm confused by this - where do you think the rejection should occur? Should we reject this when we create/add the column (and disallow, e.g.,create table t (a int not null default null)?

create table t (a int not null default null) seems to be a different case to me - then DefaultExpr == DNull and it's not nil

Really here I'm confusing myself. IT would be so much easier if the column descriptor had a method HasNullDefault().

[tyler314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/create_table.go, line 604 at r4 (raw file):

Previously, knz (kena) wrote…

Meanwhile, I think there is a missing check: we should reject SET DEFAULT unless either there is a DEFAULT expression, or there is not DEFAULT but the column is nullable.

(acknowledging that DEfaultExpr == nil means "no default" instead of "DEFAULT NULL")

The first check you mentioned makes sense to me. But for the second one, that is, if there is no DEFAULT but the column is nullable; in that case would it not be ok to set the default to NULL?

---

pkg/sql/row/cascader.go, line 734 at r4 (raw file):

Previously, knz (kena) wrote…

Out of curiosity, where's the integrity check that the cascading action is only allowed if the column is nullable? Maybe reference that in the comment.

(Or better yet: properly distinguish DefaultExpr == nil from DefaultExpr == tree.DNull, as commented above).

So in the case where DefaultExpr is nil, this means the user did a SET DEFAULT without a default expression. Should this simply not be allowed? Should we always require the user to specify a default value when using SET DEFAULT? Or only allow it if the column is nullable, and if that is the case, treat it as the same as SET NULL (which is what postgres does)?

[tyler314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, knz (kena) wrote…

create table t (a int not null default null) seems to be a different case to me - then DefaultExpr == DNull and it's not nil

Really here I'm confusing myself. IT would be so much easier if the column descriptor had a method HasNullDefault().

Is this a method worth adding?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, and @tyleroberts)

---

pkg/sql/row/cascader.go, line 734 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

So in the case where DefaultExpr is nil, this means the user did a SET DEFAULT without a default expression. Should this simply not be allowed? Should we always require the user to specify a default value when using SET DEFAULT? Or only allow it if the column is nullable, and if that is the case, treat it as the same as SET NULL (which is what postgres does)?

I thought about this a bit more, but in reality I went to check what postrgres does and I learned something.

This morning my intuition was the following (and that's what I was writing earlier):

• column is nullable and user omits DEFAULT, then we populate DefaultExpr = DNull. From then on we accept SET DEFAULT iff DefaultExpr is not nil and all is well
• column is non-nullable and user omits DEFAULT, then we populate DefaultExpr = nil. From then on we accept SET DEFAULT iff DefaultExpr is not nil and all is well
  In both cases the check for cascading actions is just defaultExpr != nil without caring for nullability at this point.

Now I think I have changed my mind. I think I see wisdom in always treating "missing DEFAULT" as "DEFAULT NULL" as you had initially envisioned.
Why I changed my mind: I found out pg does not care about the default expression when planning inserts and FK checks. This means that somehow it's not relevant.
Then I went further to investigate how come it's not relevant. Apparently the reason is that NOT NULL is checked as a constraint on the value "after all is said and done", like a CHECK constraint.
This seems to mean that it does not matter whether we let a cascading action propagate a NULL value in a non-nullable column via SET DEFAULT, because the NOT NULL constraint check, which should happen anyway, and afterwards, will catch it and abort the txn.

so if your tests demonstrate this (both that you can afford the code simplification and still the remainder of the code properly rejects nulls in non-nullable columns), I'm retracting this part of my comments.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, and @tyleroberts)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

Is this a method worth adding?

regardless of what you end up choosing, yes this would be a welcome addition. We already have a few such helper methods in structured.go.

[tyler314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, knz (kena) wrote…

regardless of what you end up choosing, yes this would be a welcome addition. We already have a few such helper methods in structured.go.

Do you think it makes sense to have two separate methods? One for checking if the default value is NULL, and one to check that there is no default value? To distinguish the difference between the cases where DefaultExpr is nil, and DefaultExpr is tree.DNull?

---

pkg/sql/row/cascader.go, line 734 at r4 (raw file):

Previously, knz (kena) wrote…

I thought about this a bit more, but in reality I went to check what postrgres does and I learned something.

This morning my intuition was the following (and that's what I was writing earlier):

• column is nullable and user omits DEFAULT, then we populate DefaultExpr = DNull. From then on we accept SET DEFAULT iff DefaultExpr is not nil and all is well
• column is non-nullable and user omits DEFAULT, then we populate DefaultExpr = nil. From then on we accept SET DEFAULT iff DefaultExpr is not nil and all is well
  In both cases the check for cascading actions is just defaultExpr != nil without caring for nullability at this point.

Now I think I have changed my mind. I think I see wisdom in always treating "missing DEFAULT" as "DEFAULT NULL" as you had initially envisioned.
Why I changed my mind: I found out pg does not care about the default expression when planning inserts and FK checks. This means that somehow it's not relevant.
Then I went further to investigate how come it's not relevant. Apparently the reason is that NOT NULL is checked as a constraint on the value "after all is said and done", like a CHECK constraint.
This seems to mean that it does not matter whether we let a cascading action propagate a NULL value in a non-nullable column via SET DEFAULT, because the NOT NULL constraint check, which should happen anyway, and afterwards, will catch it and abort the txn.

so if your tests demonstrate this (both that you can afford the code simplification and still the remainder of the code properly rejects nulls in non-nullable columns), I'm retracting this part of my comments.

Done.

[BramGruneir]

@knz, can you summarize your thoughts on all of this?

[knz]

can you summarize your thoughts on all of this?

• my first thought is that this area is devilishly hard and so I commend Tyler and Lucy for taking it on.

• my second thought is that given the complexity of the situation (and the amount of discussion it generates) the commit message is largely insufficient.

I think commit messages should have the following structure:

1. what the situation was prior
2. why the situation needs to change
3. how you're changing it and what the final state is

Additionally, when talking about compatibility with 3rd party tools, the structure of the commit message should include in step 1, whether there was a difference with the 3rd party and why, and in step 3, whether there is a difference remaining with the 3rd party and why.

The structure of this commit message is very important as it reveals the process of deciding the change and also tells the reviewers how to approach the review.

• my third and last thought is that this change may have benefited from a (mini-)RFC beforehand with lots of illustrative examples. Oh well, lesson learned.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @lucy-zhang, and @tyleroberts)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

Do you think it makes sense to have two separate methods? One for checking if the default value is NULL, and one to check that there is no default value? To distinguish the difference between the cases where DefaultExpr is nil, and DefaultExpr is tree.DNull?

If you decide to keep the equivalence between DefaultExpr == nil and DefaultExpr == DNull, a single method suffices.

If you don't, you'll want separate HasDefaultExpr() and HasNullDefault()

---

pkg/sql/create_table.go, line 604 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

The first check you mentioned makes sense to me. But for the second one, that is, if there is no DEFAULT but the column is nullable; in that case would it not be ok to set the default to NULL?

As per this comment: #39136 (review) I agree it is OK.

But then I am surprised - why not simply remove this entire check and let the cascading action go through. (I think this was your original approach in the first iteration of the PR)

If SET DEFAULT populates a null value, the NOT NULL constraint check at the end should catch it right? Have you tried?

---

pkg/sql/sqlbase/structured.go, line 2648 at r4 (raw file):

	// TODO (tyler): There should no longer be a case where the Default Expression
	// is not nil but instead NULL in 19.2. This should be removed once we are
	// confident we do not need to be concerned with supporting previous version.

I think this first TODO is inaccurate: If (for example) I write DEFAULT IF(now() < '0000-00-00', 0, NULL)
then the default value will actually be null.
The presence of a default expression (non-nil expr) does not guarantee that the evaluated value won't be NULL.

---

pkg/sql/sqlbase/structured.go, line 2658 at r4 (raw file):

			panic(errors.NewAssertionErrorWithWrappedErrf(err,
				"failed to parse default expression %s", *desc.DefaultExpr))
		} else if defaultExpr == tree.DNull {

Please add a comment below here to indicate this else branch is merely an optimization: if the expression happens to evaluate to NULL but is not a literal NULL, the backfill still does the proper job. Therefore, if the else branch here was not present, we'd still get the right behavior.

---

pkg/sql/sqlbase/table.go, line 175 at r4 (raw file):

		// properly stored, only if the default expression is not NULL.
		// Otherwise we want to keep the default expression nil.
		if typedExpr != tree.DNull {

See above: this seems to be an optimization only?
It's possible for typedExpr != tree.DNull but the result of evaluation to still be null (e.g if the expression is ore complex)

[tyler314]

I agree with everything you said. I think it makes sense to have the SET DEFAULT case behave the same way as the SET NULL. This includes preventing the creation of tables if the user sets the default to be NULL, and also sets a constraint on the columns to be non-nullable. If there is no good reason to deviate away from PG, then we follow PG. However, I think we have a good reason to deviate. There is no obvious reason why a user would ever want to do this, and preventing the user from doing so doesn't take away any control from them. Also, when this occurs while using the SET NULL case, a table is prevented from being created, this is already the behavior for 19.1. Matching this behavior while using SET DEFAULT makes the most sense for all the reasons I previously mentioned, as well as the fact it keeps the changes simpler than changing the way both these table creations are handled.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/create_table.go, line 602 at r4 (raw file):

Previously, knz (kena) wrote…

If you decide to keep the equivalence between DefaultExpr == nil and DefaultExpr == DNull, a single method suffices.

If you don't, you'll want separate HasDefaultExpr() and HasNullDefault()

Right now there is still a difference between the two, so I believe it makes sense to have two separate methods for now.

---

pkg/sql/create_table.go, line 604 at r4 (raw file):

Previously, knz (kena) wrote…

As per this comment: #39136 (review) I agree it is OK.

But then I am surprised - why not simply remove this entire check and let the cascading action go through. (I think this was your original approach in the first iteration of the PR)

If SET DEFAULT populates a null value, the NOT NULL constraint check at the end should catch it right? Have you tried?

I have not tried that. My reasoning for adding this check where it is, is to prevent the table being created in the first place. In the same way that if a user sets a NOT NULL constraint on a column and specifies SET NULL, the check immediately above this one prevents the table from being created. If it is agreed upon that we should allow users to create the tables, and allow the constraint check to catch the error, we should do so for both the SET NULL and SET DEFAULT case.

With that being said, after speaking with Bram and putting more thought into it, I think it is ok to prevent the user from creating a table in this fashion, and to deviate slightly from the way Postgres does it. If at some point someone comes up with a good reason to allow the user to create a table like this, a new issue can be opened. Although I doubt that will happen anytime soon.

---

pkg/sql/sqlbase/structured.go, line 2648 at r4 (raw file):

Previously, knz (kena) wrote…

I think this first TODO is inaccurate: If (for example) I write DEFAULT IF(now() < '0000-00-00', 0, NULL)
then the default value will actually be null.
The presence of a default expression (non-nil expr) does not guarantee that the evaluated value won't be NULL.

ok, so we will leave this check in and remove the TODO suggesting to delete it in the near future.

---

pkg/sql/sqlbase/structured.go, line 2658 at r4 (raw file):

Previously, knz (kena) wrote…

Please add a comment below here to indicate this else branch is merely an optimization: if the expression happens to evaluate to NULL but is not a literal NULL, the backfill still does the proper job. Therefore, if the else branch here was not present, we'd still get the right behavior.

I refactored this code into its own method, and removed the else statement.

---

pkg/sql/sqlbase/table.go, line 175 at r4 (raw file):

Previously, knz (kena) wrote…

See above: this seems to be an optimization only?
It's possible for typedExpr != tree.DNull but the result of evaluation to still be null (e.g if the expression is ore complex)

This code block was wrapped in this if statement to keep d.DefaultExpr.Expr nil. This is where we ensure the default expression is nil when a user explicitly sets the default value to be NULL.

[knz]

However, I think we have a good reason to deviate. [...] makes the most sense for all the reasons [...]

Then please spell out the deviations and the reasons in the commit message.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, and @tyleroberts)

---

pkg/sql/create_table.go, line 604 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

I have not tried that. My reasoning for adding this check where it is, is to prevent the table being created in the first place. In the same way that if a user sets a NOT NULL constraint on a column and specifies SET NULL, the check immediately above this one prevents the table from being created. If it is agreed upon that we should allow users to create the tables, and allow the constraint check to catch the error, we should do so for both the SET NULL and SET DEFAULT case.

With that being said, after speaking with Bram and putting more thought into it, I think it is ok to prevent the user from creating a table in this fashion, and to deviate slightly from the way Postgres does it. If at some point someone comes up with a good reason to allow the user to create a table like this, a new issue can be opened. Although I doubt that will happen anytime soon.

Ditto this can be spelled out in the commit message.

---

pkg/sql/sqlbase/structured.go, line 2648 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

ok, so we will leave this check in and remove the TODO suggesting to delete it in the near future.

Sounds good

---

pkg/sql/sqlbase/structured.go, line 2658 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

I refactored this code into its own method, and removed the else statement.

maybe forgot to push? I don't see the change.

[tyler314]

Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/create_table.go, line 604 at r4 (raw file):

Previously, knz (kena) wrote…

Ditto this can be spelled out in the commit message.

Done.

---

pkg/sql/sqlbase/structured.go, line 2648 at r4 (raw file):

Previously, knz (kena) wrote…

Sounds good

Done.

---

pkg/sql/sqlbase/structured.go, line 2658 at r4 (raw file):

Previously, knz (kena) wrote…

maybe forgot to push? I don't see the change.

I hadn't pushed yet, please take a look now.

[knz]

Yeah this commit message is much better.
A tip for next time: you can also include SQL snippets in the commit message to illustrate further.

LGTM modulo a nit in the method name

Reviewed 4 of 4 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @lucy-zhang, and @tyleroberts)

---

pkg/sql/sqlbase/structured.go, line 3241 at r5 (raw file):

// Checks that the column descriptor has no default.
func (desc *ColumnDescriptor) HasNoDefault() bool {

We don't typically use negatives in accessors as it makes the code less readable. I'd recommend renaming this to HasDefault() and have the body test DefaultExpr != nil (then switch the condition in callers).

[tyler314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/sqlbase/structured.go, line 3241 at r5 (raw file):

Previously, knz (kena) wrote…

We don't typically use negatives in accessors as it makes the code less readable. I'd recommend renaming this to HasDefault() and have the body test DefaultExpr != nil (then switch the condition in callers).

Done.

[tyler314]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, and @lucy-zhang)

---

pkg/sql/logictest/testdata/logic_test/fk, line 1222 at r1 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

I see, I think I didn't notice the ON UPDATE/ON DELETE difference and deleted the tests thinking they were redundant.

Done.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1261 at r1 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

same answer as above.

Done.

---

pkg/sql/logictest/testdata/logic_test/fk, line 1279 at r1 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

That's a good idea.

Done.

---

pkg/sql/sqlbase/structured.go, line 2648 at r1 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

I am on the fence. I am leaning to agree with you, in that having this code is misleading and really isn't necessary. On the other hand, after Jordan brought to my attention the edge case of doing a backup with the old schema, I am concerned with what could happen if I do remove the code. What do you think is best, remove it entirely?

Done.

---

pkg/sql/sqlbase/table.go, line 175 at r4 (raw file):

Previously, tyleroberts (Tyler Roberts) wrote…

This code block was wrapped in this if statement to keep d.DefaultExpr.Expr nil. This is where we ensure the default expression is nil when a user explicitly sets the default value to be NULL.

Done.

[tyler314]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)