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: make sequence integer bound consistent with default_int_size #84555

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

ZhouXing19
Copy link
Collaborator

@ZhouXing19 ZhouXing19 commented Jul 18, 2022

This PR is based on #84034.
Please just look at the 2nd commit. I'll rebase this PR once #84034 is merged.

Previously, the default bounds of sequence are alwaysmath.MaxInt64
or math.MinInt64 (depending on the sequence's order). This
can be inconsistent with the cluster setting default_int_size. This commit
is to fix it.

fixes #84554

Release note (bug fix): make sequence integer bound consistent with the
cluster setting default_int_size.

Release justification: fix a bug of sequence integer bound

@ZhouXing19 ZhouXing19 requested a review from a team July 18, 2022 03:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ZhouXing19
Copy link
Collaborator Author

Note to myself: do we need a migration for this?

@ZhouXing19 ZhouXing19 force-pushed the fix-seq-bound branch 3 times, most recently from 8b932cd to aa06bb7 Compare July 18, 2022 14:31
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

Note to myself: do we need a migration for this?

What would the migration do?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

Copy link
Collaborator

@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.

this seems like it lgtm after rebasing

@ZhouXing19
Copy link
Collaborator Author

Note to myself: do we need a migration for this?

What would the migration do?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

nvm, I misunderstood. I thought we needed to reset the existing sequences' min and max values, but it doesn't seem needed.

@ZhouXing19
Copy link
Collaborator Author

Rebased and it passed the CI. RFAL, thanks!

@ZhouXing19 ZhouXing19 requested a review from rafiss August 3, 2022 19:13
Copy link
Collaborator

@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.

just a small nit!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)


pkg/sql/information_schema.go line 486 at r5 (raw file):

				identityMax := tree.DNull
				identityMin := tree.DNull
				generatedAsIdentitySeqOpt, err := column.GetGeneratedAsIdentitySequenceOption(p.SessionData().DefaultIntSize)

in this case, i think we should not use the session variable, and should instead inspect the int size from the column type. e.g. pass in column..GetType().Width()

otherwise, the result shown in information_schema.columns would depend on the session var.

CREATE TABLE t (id1 INT GENERATED BY DEFAULT AS IDENTITY);

select identity_maximum from information_schema.columns where table_name = 't' and column_name='id1';

set default_int_size = 4;

select identity_maximum from information_schema.columns where table_name = 't' and column_name='id1';

we'd want the two selects to return the same value

@ZhouXing19
Copy link
Collaborator Author

in this case, i think we should not use the session variable, and should instead inspect the int size from the column type. e.g. pass in column..GetType().Width()

Makes sense. Made the change and updated the tests.

@ZhouXing19 ZhouXing19 requested a review from rafiss August 15, 2022 17:12
Copy link
Collaborator

@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.

lgtm, nice!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @ZhouXing19)

@ZhouXing19
Copy link
Collaborator Author

TFTR!
bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Aug 15, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 15, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 15, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 15, 2022

Build failed:

Previously, the default bound of sequence are always
`math.MaxInt64` or `math.MinInt64` (depending on the sequence's order). This
is inconsistent with the cluster setting `default_int_size`. This commit is
to fix it.

fixes cockroachdb#84554

Release note (bug fix): make sequence integer bound consistent with the
cluster setting `default_int_size`.

Release justification: bug fix
@ZhouXing19
Copy link
Collaborator Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 18, 2022

Build succeeded:

@craig craig bot merged commit 5789fac into cockroachdb:master Aug 18, 2022
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: sequence integer bounds do not correspond to default_int_size
4 participants