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

ON UPDATE expressions #12035

Merged
merged 6 commits into from
Nov 3, 2021
Merged

ON UPDATE expressions #12035

merged 6 commits into from
Nov 3, 2021

Conversation

ericharmeling
Copy link
Contributor

Fixes #11441.
Related to #11028.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@netlify
Copy link

netlify bot commented Oct 21, 2021

✔️ Netlify Preview

🔨 Explore the source changes: 4da219e

🔍 Inspect the deploy log: https://app.netlify.com/sites/cockroachdb-docs/deploys/61827552846a1b000851a002

😎 Browse the preview: https://deploy-preview-12035--cockroachdb-docs.netlify.app/docs/v21.2/add-column

@ericharmeling
Copy link
Contributor Author

@mgartner Adding you for eng review, as original author of cockroachdb/cockroach#69091 is no longer with us.

Feel free to request a reassign!

@rafiss
Copy link
Contributor

rafiss commented Oct 25, 2021

one thought: in the computed columns page, we should also mention that computed columns are mutually exclusive with ON UPDATE. (this consideration is already noted for DEFAULT)

https://www.cockroachlabs.com/docs/stable/computed-columns.html#considerations

@ericharmeling
Copy link
Contributor Author

@rmloveland

@ericharmeling
Copy link
Contributor Author

in the computed columns page, we should also mention that computed columns are mutually exclusive with ON UPDATE. (this consideration is already noted for DEFAULT)

Done!

rmloveland added a commit that referenced this pull request Oct 26, 2021
Fixes #11214
Fixes #11454
Fixes #11212

Depends on a link that will exist once we merge #12035
rmloveland added a commit that referenced this pull request Oct 26, 2021
Fixes #11214
Fixes #11454
Fixes #11212

Links to ON UPDATE content that will exist once we merge #12035
Copy link
Contributor

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

examples looks great! just had comments about how DEFAULT and ON UPDATE should probably be categorized the same as each other

@@ -21,7 +21,7 @@ Computed columns:
- Cannot be used to generate other computed columns.
- Cannot reference a [foreign key](foreign-key.html).
- Behave like any other column, with the exception that they cannot be written to directly.
- Are mutually exclusive with [`DEFAULT`](default-value.html).
- Are mutually exclusive with [`DEFAULT` constraints](default-value.html) and [`ON UPDATE` expressions](add-column.html#on-update-expressions).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: DEFAULT isn't really a "constraint" -- maybe say DEFAULT expressions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or DEFAULT qualifications?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT isn't really a "constraint"

Hmm... we document DEFAULT as a column constraint all over our docs (see the Default Value Constraint and Constraints pages).

I've updated this particular instance to use "DEFAULT expressions", but, if you think the other docs are incorrect/misleading, I can file a new issue to update how we document DEFAULT in a separate PR. Maybe we just fold the entire "Default Value Constraint" page into a subsection of ADD COLUMN (like we've done here with ON UPDATE).

- [Column-level constraints](constraints.html)
- [Collations](collate.html)
- [Column family assignments](column-families.html)
- <span class="version-tag">New in v21.2</span>: [`ON UPDATE` expressions](#on-update-expressions)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to my other comment, i don't thing DEFAULT should be included under constraints. instead, DEFAULT should be documented as basically identically to ON UPDATE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

(but see my comment below)

Copy link
Contributor

@mgartner mgartner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

@@ -21,7 +21,7 @@ Computed columns:
- Cannot be used to generate other computed columns.
- Cannot reference a [foreign key](foreign-key.html).
- Behave like any other column, with the exception that they cannot be written to directly.
- Are mutually exclusive with [`DEFAULT`](default-value.html).
- Are mutually exclusive with [`DEFAULT` constraints](default-value.html) and [`ON UPDATE` expressions](add-column.html#on-update-expressions).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DEFAULT isn't really a "constraint"

Hmm... we document DEFAULT as a column constraint all over our docs (see the Default Value Constraint and Constraints pages).

I've updated this particular instance to use "DEFAULT expressions", but, if you think the other docs are incorrect/misleading, I can file a new issue to update how we document DEFAULT in a separate PR. Maybe we just fold the entire "Default Value Constraint" page into a subsection of ADD COLUMN (like we've done here with ON UPDATE).

Note the following limitations of `ON UPDATE` expressions:

- You cannot add a [foreign key constraint](foreign-key.html) and an `ON UPDATE` expression to the same column.
- Values populated by [`DEFAULT` constraints](default-value.html) do not trigger an `ON UPDATE` change.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Values populated by DEFAULT constraints do not trigger an ON UPDATE change.

Updated this line

- [Column-level constraints](constraints.html)
- [Collations](collate.html)
- [Column family assignments](column-families.html)
- <span class="version-tag">New in v21.2</span>: [`ON UPDATE` expressions](#on-update-expressions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

(but see my comment below)

@ericharmeling ericharmeling requested a review from rafiss October 28, 2021 15:36
rmloveland added a commit that referenced this pull request Oct 28, 2021
Fixes #11214
Fixes #11454
Fixes #11212

Links to ON UPDATE content that will exist once we merge #12035
@pawalt
Copy link

pawalt commented Oct 28, 2021

This looks great! My only note is that ON UPDATE expressions have some behavior regarding when they execute that might be surprising if coming from MySQL ON UPDATE. Probably worth documenting:
https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20210722_on_update_and_row_rehoming.md#when-does-on-update-execute

It also may be worth noting that ON UPDATE expressions are closest to DEFAULT in terms of what kinds of expressions they allow (they allow context-dependent expressions but not expressions that reference other columns). For ex. current_timestamp() is allowed.

rmloveland added a commit that referenced this pull request Nov 1, 2021
Fixes #11214
Fixes #11454
Fixes #11212

Links to ON UPDATE content that will exist once we merge #12035
@ericharmeling
Copy link
Contributor Author

@pawalt

Thanks for taking the time to look over this! I've rewritten the "ON UPDATE expressions" section, following your feedback. I am a bit uncertain about the ON UPDATE CASCADE case - could you verify that what I've written is correct?

@pawalt
Copy link

pawalt commented Nov 2, 2021

@ericharmeling LGTM!

@ericharmeling ericharmeling requested a review from a user November 2, 2021 22:46
- [`DEFAULT` expressions](default-value.html)
- <span class="version-tag">New in v21.2</span>: [`ON UPDATE` expressions](#on-update-expressions)

### ON UPDATE expressions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
### ON UPDATE expressions
### `ON UPDATE` expressions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See suggestion for formatting in heading.

@ericharmeling ericharmeling merged commit 4f44bfc into master Nov 3, 2021
@ericharmeling ericharmeling deleted the on-update branch November 3, 2021 12:13
rmloveland added a commit that referenced this pull request Nov 4, 2021
Fixes #11214
Fixes #11454
Fixes #11212

Links to ON UPDATE content that will exist once we merge #12035
rmloveland added a commit that referenced this pull request Nov 8, 2021
Fixes #11214
Fixes #11454
Fixes #11212

Also links to new ON UPDATE content that was added in #12035
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: implement ON UPDATE expressions
5 participants