-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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/delegate: include composite types in SHOW TYPES #124730
sql/delegate: include composite types in SHOW TYPES #124730
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! the fix looks good to me, but i just left a minor comment on how to do the database lookup
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/delegate/show_types.go
line 27 at r1 (raw file):
// call ResolveSchema with an empty schema to specifically retrieve current // database/schema name. _, name, err := d.catalog.ResolveSchema(d.ctx, flags, &cat.SchemaName{})
nit: we can access the current database in a slightly easier way that avoids performing a lookup. instead, we can look at the in-memory session data, like this:
cockroach/pkg/sql/delegate/show_grants.go
Line 235 in 48233b4
currDB := d.evalCtx.SessionData().Database |
pkg/sql/logictest/testdata/logic_test/composite_types
line 274 at r1 (raw file):
public et1 root sc1 ct2 root sc1 ct3 root
nice test!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/delegate/show_types.go
line 27 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: we can access the current database in a slightly easier way that avoids performing a lookup. instead, we can look at the in-memory session data, like this:
cockroach/pkg/sql/delegate/show_grants.go
Line 235 in 48233b4
currDB := d.evalCtx.SessionData().Database
Much simpler.
12cbacc
to
fdb0fd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
pkg/sql/delegate/show_types.go
line 42 at r2 (raw file):
nsp.nspname NOT IN ('information_schema', 'pg_catalog', 'crdb_internal', 'pg_extension') ORDER BY (nsp.nspname, types.typname) `, d.evalCtx.SessionData().Database)
i just remembered another tricky edge case that matters here. if the name of an identifier has upper-case letters or if the name has certain characters such as spaces, "
, or -
, then the name needs to be double-quoted for it to be parsed correctly. there are two ways this can be achieved:
tree.Name(d.evalCtx.SessionData().Database).String()
lexbase.EscapeSQLIdent(d.evalCtx.SessionData().Database)
The first option just ends up calling in to the second option. I think within the delegate
package, the first option might be more conventional to use.
a test could be added for this in the logic tests with something like:
CREATE DATABASE "MyDatabase";
USE "MyDatabase";
CREATE TYPE ct5 AS (a INT, b TEXT);
CREATE TYPE ct6 AS ENUM ('a', 'b', 'c');
SHOW TYPES
sorry for not remembering this earlier!
SHOW TYPES previously only included the enumerated types. In 23.1 composite types were added, but we forgot to update SHOW TYPES. This corrects that by including composite types in the catalog lookup. I query pg_catalog.pg_type to get the types. But there are a bunch of internal types in their that need to be omitted: - anything from any of the known internal schemas are skipped: information_schema, pg_catalog, crdb_internal, and pg_extension. - each time a table is created, a type for the table descriptor is added. I query typrelid to filter out the table descriptor. It must be 0, for enums, or refer back to its own oid so that we know its a standalone type. Fixes: cockroachdb#119734 Release note (bug fix): SHOW TYPES includes user defined composite types. It omitted those types ever since composite types were added in 23.1.
fdb0fd5
to
75d168c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/sql/delegate/show_types.go
line 42 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
i just remembered another tricky edge case that matters here. if the name of an identifier has upper-case letters or if the name has certain characters such as spaces,
"
, or-
, then the name needs to be double-quoted for it to be parsed correctly. there are two ways this can be achieved:
tree.Name(d.evalCtx.SessionData().Database).String()
lexbase.EscapeSQLIdent(d.evalCtx.SessionData().Database)
The first option just ends up calling in to the second option. I think within the
delegate
package, the first option might be more conventional to use.a test could be added for this in the logic tests with something like:
CREATE DATABASE "MyDatabase"; USE "MyDatabase"; CREATE TYPE ct5 AS (a INT, b TEXT); CREATE TYPE ct6 AS ENUM ('a', 'b', 'c'); SHOW TYPES
sorry for not remembering this earlier!
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! when you're ready feel free to merge using bors
this change is a bug fix, so it's also something we should backport to older release branches, so the fix goes out in patch releases for existing versions. we should backport to 23.1, 23.2, and 24.1, since those are the current supported releases.
this page describes 3 different options for how to do a backport: https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/900005932/Backporting+a+change+to+a+release+branch. (the last option is required if there are any merge conflicts.) also, we can discuss this more during our 1-1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)
bors r=rafiss |
SHOW TYPES previously only included the enumerated types. In 23.1 composite types were added, but we forgot to update SHOW TYPES. This corrects that by including composite types in the catalog lookup.
Unlike other SHOW statements, there is no way to filter out schema. So, this will return composite types for all schemas. We do a lookup to ResolveSchema, knowing it won't find a schema, in order to get the current database name.
I query pg_catalog.pg_type to get the types. But there are a bunch of internal types in their that need to be omitted:
Fixes: #119734
Release note (bug fix): SHOW TYPES includes user defined composite types. It omitted those types ever since composite types were added in 23.1.