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: code to check whether a sequence exists is bad #72820

Closed
ajwerner opened this issue Nov 16, 2021 · 0 comments · Fixed by #118861
Closed

sql: code to check whether a sequence exists is bad #72820

ajwerner opened this issue Nov 16, 2021 · 0 comments · Fixed by #118861
Assignees
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. A-sql-name-resolution SQL name resolution rules and CTEs. A-sql-sequences Sequence handling in SQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajwerner
Copy link
Contributor

ajwerner commented Nov 16, 2021

Describe the problem

The code to check whether a sequence exists uses descriptor resolution APIs which indicate a name is not in use when it's in use by a type.

res, err := p.resolveUncachedTableDescriptor(ctx, seqName, false /*required*/, tree.ResolveAnyTableKind)
if err != nil {
return nil, nil, nil, nil, err
}

To Reproduce

set serial_normalization = 'sql_sequence';
create type t_t_seq AS enum ('a');
create table t (t SERIAL PRIMARY KEY);

Leads to this inscrutible error:

ERROR: unexpected value: raw_bytes:"\304\244\321\003\001v" timestamp:<wall_time:1637082444028995000 > 

Expected behavior
We call the sequence t_t_seq1

Additional context
The deeper problem here is that we lack primitives to ask whether an object (postgres would say relation) exists with a given name without specifying what type of object we want. This problem lies here:

// DesiredObjectKind represents what kind of object is desired in a name
// resolution attempt.
type DesiredObjectKind int
const (
// TableObject is used when a table-like object is desired from resolution.
TableObject DesiredObjectKind = iota
// TypeObject is used when a type-like object is desired from resolution.
TypeObject
)

We probably need some sort of AnyObject here. That's all fine and good, but it gets a tad tricky here when we want to construct a name for the purposes of errors.

// NewQualifiedObjectName returns an ObjectName of the corresponding kind.
// It is used mainly for constructing appropriate error messages depending
// on what kind of object was requested.
func NewQualifiedObjectName(catalog, schema, object string, kind DesiredObjectKind) ObjectName {

At a deeper level, I don't really understand what value we get out of having different types for TableName and TypeName.

Jira issue: CRDB-11305

Epic CRDB-35306

@ajwerner ajwerner added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-sequences Sequence handling in SQL A-sql-name-resolution SQL name resolution rules and CTEs. A-schema-descriptors Relating to SQL table/db descriptor handling. labels Nov 16, 2021
@ajwerner ajwerner added this to Triage in SQL Foundations via automation Nov 16, 2021
@blathers-crl blathers-crl bot added the T-sql-schema-deprecated Use T-sql-foundations instead label Nov 16, 2021
ajwerner added a commit to ajwerner/cockroach that referenced this issue Nov 23, 2021
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.
@ajwerner ajwerner moved this from Triage to General 22.1 in SQL Foundations Nov 23, 2021
@ajwerner ajwerner self-assigned this Nov 23, 2021
ajwerner added a commit to ajwerner/cockroach that referenced this issue Nov 24, 2021
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.
@postamar postamar moved this from General 22.1 to Backlog in SQL Foundations Mar 8, 2022
@ajwerner ajwerner removed their assignment Jul 5, 2022
@postamar postamar moved this from Backlog to Tech Debt That Nobody Outside Of The Team Cares About in SQL Foundations Nov 10, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
chrisseto added a commit to chrisseto/cockroach that referenced this issue Dec 20, 2023
This commit adds support for attaching comments to various schema
elements in the RSW. Notably, commenting on databases has been omitted
as the workload operates within the scope of a given database.

Unfortunately, `COMMENT ON` will consistently trigger the bug reported
in cockroachdb#72820. Therefore it is disabled by default.

Epic: CRDB-19168
Informs: CRDB-3265
Informs: cockroachdb#72820
Release note: None
@rafiss rafiss self-assigned this Feb 7, 2024
rafiss added a commit to rafiss/cockroach that referenced this issue Feb 7, 2024
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.
rafiss added a commit to rafiss/cockroach that referenced this issue Feb 7, 2024
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.
craig bot pushed a commit that referenced this issue Feb 8, 2024
118781: sql: add server.max_open_transactions_per_gateway cluster setting r=rafiss a=rafiss

informs #110272
Release note (sql change): Added the
server.max_open_transactions_per_gateway cluster setting. When set to a non-negative value, then non-admin users cannot execute a query if the number of transactions open on the current gateway node is already at the configured limit.

118861: sql: fix bug with type name conflict, allow resolving any object r=rafiss a=rafiss

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.

Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
@craig craig bot closed this as completed in bac933e Feb 8, 2024
SQL Foundations automation moved this from Tech Debt That Nobody Outside Of The Team Cares About to Done Feb 8, 2024
cockroach-dev-inf pushed a commit that referenced this issue Feb 8, 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

Fixes #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.
cockroach-dev-inf pushed a commit that referenced this issue Feb 8, 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

Fixes #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.
wenyihu6 pushed a commit to wenyihu6/cockroach that referenced this issue Feb 21, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-schema-descriptors Relating to SQL table/db descriptor handling. A-sql-name-resolution SQL name resolution rules and CTEs. A-sql-sequences Sequence handling in SQL C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
3 participants