sql: add ON UPDATE syntax by pawalt · Pull Request #68803 · cockroachdb/cockroach

pawalt · 2021-08-12T16:10:29Z

Previously, we did not have parsing implemented for ON UPDATE
expressions on columns. This PR adds parsing for ON UPDATE expressions.

Release note: None

cockroach-teamcity · 2021-08-12T16:10:35Z

This change is

otan

oops, missed a comment.

otan · 2021-08-13T00:04:27Z

pkg/sql/parser/sql.y

  }
+| ON UPDATE b_expr
+  {
+    $$.val = &tree.ColumnOnUpdate{Expr: $3.expr()}


are we dealing with constraint name? there's some code here that assumes we are.

Yes; because of the way the column qualifications are parsed, both the anonymous syntax ON UPDATE expr and the named syntax CONSTRAINT name ON UPDATE expr are valid uses.

You should add a test in the Parser for this

otan · 2021-08-13T00:09:21Z

i know this is late, but this FK stuff has be wondering - what we should do when we have col INT ON UPDATE x REFERENCES other_table(col) ON UPDATE SET NULL, and other_table_col(col) gets updated.

pawalt · 2021-08-13T15:14:16Z

@otan My interpretation is that the ON UPDATE x would be applied in that case. I think this falls under the same logic as ON UPDATE CASCADE. Any modification to a column results in the ON UPDATE being applied.

otan · 2021-08-13T21:21:45Z

I think we should only have one ON UPDATE in the clause at any time, and/or prohibit FK ON UPDATE and ON UPDATE in the same column qualification.

Happy for that to be a separate PR.

However, I feel like that also has knock on effects for parsing in that if you can guarantee one ON UPDATE in a qualification you can eliminate the ambiguity of ON UPDATE for FKs or ON UPDATE as a trigger. You can say if a FK is in the clause, the ON UPDATE bit is for the FK and only allow DEFAULT AND NULL as the expr, otherwise its the ON UPDATE trigger. That way we can keep the ON UPDATE SET clause. What do you think about this?

If you feel strongly about making this just ON UPDATE and we are deviating from the RFC, we should update it accordingly.

pawalt · 2021-08-16T14:50:23Z

The multiple ON UPDATE concern should be taken care of by step where I build the ColumnTableDef. I check if there are multiple ON UPDATEs and error out if there is one.

I agree about prohibiting a FK ON UPDATE and an ON UPDATE in the same clause, but I think using that restriction to make parsing easier would be tough. From the parser perspective, the ON UPDATE reference action would have to be lifted to a top-level qualification, making it harder to construct a FK node since the ON UPDATE sits at the same level as the FK node.

This change would also make it impossible to specify an ON UPDATE expression with a value of DEFAULT/NULL for foreign key columns. To be clear, I don't think that this is a compelling use for ON UPDATE; I just don't like the semantics changing based on another qualification.

I don't have strong opinions on ON UPDATE from a SQL standards perspective, but I think in this case it makes behavior more explicit, and it's not a significant departure from ON UPDATE SET.

Once this PR gets merged in, I'll update the RFC with whatever gets decided here.

otan · 2021-08-16T21:26:43Z

pkg/sql/parser/sql.y


 reference_on_delete:
-  ON DELETE reference_action
+  ON_DELETE DELETE reference_action


can you name this ON_DELETE_LA

Changed to ON_LA

otan · 2021-08-16T21:29:28Z

docs/generated/sql/bnf/stmt_block.bnf


 reference_on_update ::=
-	'ON' 'UPDATE' reference_action
+	'ON_UPDATE' 'UPDATE' reference_action


sigh, i rarely read generated files on reviews but maybe i should
sorry to spring this on you also, but do you know why this isn't ON_UPDATE_LA?

hmm, i guess the generated code omits the _LA as the other ones don't seem to be mentioned here.
maybe to make this clearer, it should be ON_FK_UPDATE

thinking about this more, this might actually have to be just ON_LA so that the bnfs we use in our railroad diagrams in our docs just show ON. or we change the generated code to make this nicer...

Oh good call. I just have to disambiguate between FK and ON UPDATE, so ON_LA for the FK use cases works here.

you still need a make buildshort to update this

otan · 2021-08-17T00:53:57Z

pkg/sql/parser/lexer.go

+		case ON:
+			switch nextID {
+			case DELETE:
+				lval.id = ON_DELETE


can you keep this as ON, or does it shift-reduce conflict?

This ends up in conflict. From my testing, I always have to disambiguate the usage of ON or else I get a conflict.

otan · 2021-08-17T20:09:20Z

docs/generated/sql/bnf/stmt_block.bnf


 reference_on_update ::=
-	'ON' 'UPDATE' reference_action
+	'ON_UPDATE' 'UPDATE' reference_action


you still need a make buildshort to update this

otan · 2021-08-17T20:10:31Z

pkg/sql/parser/testdata/create_table

+error
+CREATE TABLE foo(b INT8 ON UPDATE 1 REFERENCES blah (blah) ON UPDATE SET NULL)
+----
+at or near ")": syntax error: ON UPDATE expression and ON UPDATE foreign key action both specified for column "b"


we also need to do this for ALTER TABLE ADD FOREIGN KEY / SET ON UPDATE. but we can do that in a separate commit (please make an issue)

Yep I'm planning on adding this in the next PR since you can't just do this at the parsing level. #69052

Previously, we did not have parsing implemented for ON UPDATE expressions on columns. This PR adds parsing for ON UPDATE expressions. Release note: None

pawalt · 2021-08-17T20:21:12Z

bors r=otan

ajwerner

Can I get confirmation that we don't yet actually allow these clauses to be specified anywhere yet? Do we have reasonable gating around this feature? Have we talked about:

Updating the catalog.Column interface?
What can be referenced in these expressions?
- Are we properly tracking them for uses of things like enum types?
Validating the descriptors with these things?

I feel like I'm not at all sure about what's going into this feature and I know there's been an RFC but it's been busy. I think I need to catch up. What's the status of this project. Are the work items broken out somewhere in a tracking issue?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani, @otan, and @pawalt)

pawalt · 2021-08-17T22:22:34Z

@ajwerner These clauses can be specified, but I don't think we need gating; in the morning I'll put up a PR implementing the logic for ON UPDATE. For your questions:

Yes, HasOnUpdate and GetOnUpdateExpr functions will have to be added.
References in ON UPDATE expressions follow the same rules as DEFAULT statements i.e. volatile expressions are allowed, but referencing other columns is not.
I'm adding validation logic to ensure that an ON UPDATE expression is not specified on a column with a foreign key update action. This is because it's ambiguous whether to apply the ON UPDATE expr or apply the FK action when a FK update happens.

Regarding issues, I haven't done a good job of tracking this, but I've thrown these up: #69052, #69057, #69058.

craig · 2021-08-17T23:48:18Z

Build succeeded:

GitHub CI (Cockroach)

pawalt requested review from arulajmani and otan August 12, 2021 16:10

pawalt force-pushed the on_update_syntax branch from 5964937 to 572ad64 Compare August 12, 2021 16:13

otan approved these changes Aug 12, 2021

View reviewed changes

otan suggested changes Aug 13, 2021

View reviewed changes

pawalt requested a review from otan August 13, 2021 16:06

otan reviewed Aug 16, 2021

View reviewed changes

otan reviewed Aug 17, 2021

View reviewed changes

pawalt force-pushed the on_update_syntax branch from 572ad64 to 3945cf5 Compare August 17, 2021 13:55

pawalt requested a review from a team August 17, 2021 13:55

blathers-crl bot requested a review from otan August 17, 2021 13:55

pawalt force-pushed the on_update_syntax branch from 3945cf5 to 8916eea Compare August 17, 2021 19:55

otan approved these changes Aug 17, 2021

View reviewed changes

pawalt mentioned this pull request Aug 17, 2021

sql: prevent use of foreign key ON UPDATE and ON UPDATE expressions on the same column #69052

Closed

sql: add ON UPDATE syntax

89d467a

Previously, we did not have parsing implemented for ON UPDATE expressions on columns. This PR adds parsing for ON UPDATE expressions. Release note: None

pawalt force-pushed the on_update_syntax branch from 8916eea to 89d467a Compare August 17, 2021 20:16

ajwerner reviewed Aug 17, 2021

View reviewed changes

pawalt mentioned this pull request Aug 17, 2021

sql: implement ON UPDATE in optimizer #69057

Closed

craig bot merged commit 5d2c91c into cockroachdb:master Aug 17, 2021

Conversation

pawalt commented Aug 12, 2021

Uh oh!

cockroach-teamcity commented Aug 12, 2021

Uh oh!

otan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

otan commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pawalt commented Aug 13, 2021

Uh oh!

otan commented Aug 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pawalt commented Aug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawalt commented Aug 17, 2021

Uh oh!

ajwerner left a comment

Choose a reason for hiding this comment

Uh oh!

pawalt commented Aug 17, 2021

Uh oh!

craig bot commented Aug 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

otan commented Aug 13, 2021 •

edited

Loading

otan commented Aug 13, 2021 •

edited

Loading