-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: add ON UPDATE syntax #68803
sql: add ON UPDATE syntax #68803
Conversation
5964937
to
572ad64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, missed a comment.
@@ -6708,6 +6725,10 @@ col_qualification_elem: | |||
{ | |||
$$.val = &tree.ColumnDefault{Expr: $2.expr()} | |||
} | |||
| ON UPDATE b_expr | |||
{ | |||
$$.val = &tree.ColumnOnUpdate{Expr: $3.expr()} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we dealing with constraint name? there's some code here that assumes we are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a test in the Parser for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
i know this is late, but this FK stuff has be wondering - what we should do when we have |
@otan My interpretation is that the |
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. |
The multiple ON UPDATE concern should be taken care of by step where I build the 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. |
pkg/sql/parser/sql.y
Outdated
{ | ||
$$.val = $3.referenceAction() | ||
} | ||
|
||
reference_on_delete: | ||
ON DELETE reference_action | ||
ON_DELETE DELETE reference_action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you name this ON_DELETE_LA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to ON_LA
@@ -3044,10 +3050,10 @@ family_name ::= | |||
name | |||
|
|||
reference_on_update ::= | |||
'ON' 'UPDATE' reference_action | |||
'ON_UPDATE' 'UPDATE' reference_action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh good call. I just have to disambiguate between FK and ON UPDATE, so ON_LA for the FK use cases works here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you still need a make buildshort
to update this
pkg/sql/parser/lexer.go
Outdated
case ON: | ||
switch nextID { | ||
case DELETE: | ||
lval.id = ON_DELETE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you keep this as ON
, or does it shift-reduce conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up in conflict. From my testing, I always have to disambiguate the usage of ON or else I get a conflict.
572ad64
to
3945cf5
Compare
3945cf5
to
8916eea
Compare
@@ -3044,10 +3050,10 @@ family_name ::= | |||
name | |||
|
|||
reference_on_update ::= | |||
'ON' 'UPDATE' reference_action | |||
'ON_UPDATE' 'UPDATE' reference_action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you still need a make buildshort
to update this
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
8916eea
to
89d467a
Compare
bors r=otan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@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:
Regarding issues, I haven't done a good job of tracking this, but I've thrown these up: #69052, #69057, #69058. |
Build succeeded: |
Previously, we did not have parsing implemented for ON UPDATE
expressions on columns. This PR adds parsing for ON UPDATE expressions.
Release note: None