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: Add support for altering sequence options identity columns #119432

Merged
merged 1 commit into from Mar 14, 2024

Conversation

andrew-delph
Copy link
Contributor

Informs: #110010

This change introduces the 'ALTER [COLUMN] column_name SET sequence_option | RESTART [ [ WITH ] restart ] }' command. This enhancement allows for the modification of identity column sequence options, thereby providing more precise control over the behavior of identity columns.

Release note (sql change): Identity columns now support enhanced sequence management through the 'ALTER [COLUMN] column_name SET sequence_option' and 'ALTER [COLUMN] column_name RESTART [WITH restart]' commands. This update facilitates the fine-tuning of identity column sequences.

@andrew-delph andrew-delph requested review from a team as code owners February 20, 2024 22:41
Copy link

blathers-crl bot commented Feb 20, 2024

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Feb 20, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrew-delph
Copy link
Contributor Author

Here is a screenshot of the new parser diagrams:

image
image

Copy link

blathers-crl bot commented Feb 21, 2024

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@andrew-delph andrew-delph force-pushed the alter_identity_opts branch 3 times, most recently from d29ebb6 to 64f61ae Compare February 22, 2024 17:30
@rafiss rafiss requested a review from fqazi March 11, 2024 17:09
Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Great work @andrew-delph and thanks for doing this. Its fairly close just some minor comments / nits.

Reviewed 10 of 12 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andrew-delph)


pkg/sql/alter_table.go line 1071 at r1 (raw file):

		// It is assummed that an identiy column owns only one sequence.
		if col.NumUsesSequences() != 1 {
			return colinfo.NewUndefinedColumnError(string(col.ColName()))

I don't think this should happen so lets use errors.AssertionFailed if we see it.


pkg/sql/alter_table.go line 1088 at r1 (raw file):

		opts := seqDesc.GetSequenceOpts()
		f := tree.NewFmtCtx(tree.FmtSimple)
		f.Printf(" MINVALUE %d", opts.MinValue)

Might be cleaner to just make this out of tree.SequenceOption and then format it out


pkg/sql/alter_table.go line 1096 at r1 (raw file):

Previously, andrew-delph (Andrew Delph) wrote…

Is it possible that 'GeneratedAsIdentitySequenceOption' can be overridden by another transaction?
It will be parsed down stream when writing to the information_schema.

So other concurrent writes can happen, but your transaction will be forced to retry because of the conflict. So, theres no danger and this will work correctly.


pkg/sql/logictest/testdata/logic_test/information_schema line 2976 at r1 (raw file):


statement ok
CREATE DATABASE test_identity

You can just use the same database for this test, no need to make a new one

Informs: cockroachdb#110010

This change introduces the 'ALTER [COLUMN] column_name SET
sequence_option | RESTART [ [ WITH ] restart ] }' command.
This enhancement allows for the modification of identity column
sequence options, thereby providing more precise control over
the behavior of identity columns.

Release note (sql change): Identity columns now support enhanced
sequence management through the 'ALTER [COLUMN] column_name SET
sequence_option' and 'ALTER [COLUMN] column_name RESTART [WITH
restart]' commands. This update facilitates the fine-tuning of
identity column sequences.
@andrew-delph
Copy link
Contributor Author

andrew-delph commented Mar 13, 2024

Thank you @fqazi for the comments!
I've made some changes.

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 12 of 12 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andrew-delph)

@fqazi
Copy link
Collaborator

fqazi commented Mar 14, 2024

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 14, 2024

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 14, 2024

@craig craig bot merged commit db58abe into cockroachdb:master Mar 14, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants