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: fix bug with type name conflict, allow resolving any object #118861

Merged
merged 3 commits into from Feb 8, 2024

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Feb 6, 2024

Prior to this change there was a bug when a type name conflicted with a sequence name (#72820). This resolves that problem by adding logic to resolve any sort of object with a given name.

It also removes a bad method from sql/resolver.go

replaces #72824
fixes #72820
fixes #118753
fixes #116795

Release note (bug fix): Fixed a bug which caused an inscrutable error when a sequence name allocated by SERIAL conflicted with an existing type name.

@rafiss rafiss added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Feb 6, 2024
@rafiss rafiss requested a review from fqazi February 6, 2024 21:54
@rafiss rafiss requested review from a team as code owners February 6, 2024 21:54
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link

blathers-crl bot commented Feb 7, 2024

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

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

@rafiss rafiss requested a review from a team as a code owner February 7, 2024 14:17
@rafiss rafiss requested review from herkolategan and srosenberg and removed request for a team February 7, 2024 14:17
@rafiss rafiss force-pushed the fix-name-obj-type branch 4 times, most recently from 9b47049 to 447d33c Compare February 7, 2024 16:40
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.

Reviewed 14 of 14 files at r1, 1 of 1 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @rafiss, and @srosenberg)


pkg/sql/serial.go line 96 at r4 (raw file):

		RequireMutable:    false,
		IncludeOffline:    true,
		DesiredObjectKind: tree.AnyObject,

We need a similar change to the declarative CREATE SEQUENCE. It attempts to only resolve a table when it needs to check for any conflict.

i.e.:
create type t_t_seq AS enum ('a');
create sequence t_t_seq;

@rafiss rafiss force-pushed the fix-name-obj-type branch 3 times, most recently from 7648004 to 3564caf Compare February 7, 2024 21:33
@rafiss rafiss requested a review from fqazi February 7, 2024 21:49
Copy link
Collaborator Author

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @herkolategan, and @srosenberg)


pkg/sql/serial.go line 96 at r4 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

We need a similar change to the declarative CREATE SEQUENCE. It attempts to only resolve a table when it needs to check for any conflict.

i.e.:
create type t_t_seq AS enum ('a');
create sequence t_t_seq;

nice catch. fixed

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, and thanks for doing this :lgtm:

Reviewed 2 of 3 files at r3, 2 of 2 files at r5, 1 of 1 files at r6, 5 of 5 files at r7, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan and @srosenberg)

Prior to this change there was a bug when a type name conflicted with a
sequence name (cockroachdb#72820). This resolves that problem by adding logic to resolve
any sort of object with a given name.

It also removes a bad method from sql/resolver.go

Fixes cockroachdb#72820.

Release note (bug fix): Fixed a bug which caused an inscrutable error when a
sequence name allocated by `SERIAL` conflicted with an existing type name.
Update the name resolution logic so that type names are checked when
creating a sequence.

No release note since this hasn't been released yet.

Release note: None
@rafiss
Copy link
Collaborator Author

rafiss commented Feb 8, 2024

tftr!

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 8, 2024

Build succeeded:

@craig craig bot merged commit 7074888 into cockroachdb:master Feb 8, 2024
8 of 9 checks passed
Copy link

blathers-crl bot commented Feb 8, 2024

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from bac933e to blathers/backport-release-23.1-118861: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


error creating merge commit from 60171b4 to blathers/backport-release-23.2-118861: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
3 participants