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: include DB name in create_statement in create_type_statements #137580

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Dec 17, 2024

This commit updates how we populate crdb_internal.create_type_statements virtual table so that create_statement column included the DB name (previously only the schema and the UDT name were included). This will make it easier to reproduce stmt bundles with cross-DB references.

Also, this commit fixes how we use this virtual table during bundle creation - we now properly use "".crdb_internal search path and cast the type oid to OID type. The same fix is applied for create_function_statements and create_procedure_statements vtables.

Epic: None
Release note: None

@yuzefovich yuzefovich requested review from Dedej-Bergin and a team December 17, 2024 02:23
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich force-pushed the udt-internal branch 3 times, most recently from 7757d42 to e71e158 Compare December 17, 2024 20:21
@yuzefovich yuzefovich changed the title sql: show correct DB in crdb_internal.create_type_statements sql: include DB name in create_statement in create_type_statements Dec 17, 2024
@yuzefovich yuzefovich requested a review from rafiss December 17, 2024 20:22
This commit updates how we populate
`crdb_internal.create_type_statements` virtual table so that
`create_statement` column included the DB name (previously only the
schema and the UDT name were included). This will make it easier to
reproduce stmt bundles with cross-DB references.

Also, this commit fixes how we use this virtual table during bundle
creation - we now properly use `"".crdb_internal` search path and cast
the type oid to `OID` type. The same fix is applied for
`create_function_statements` and `create_procedure_statements` vtables.

Release note: None
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! thanks for this improvement. is there anything remaining to resolve #137579 now?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

@yuzefovich
Copy link
Member Author

TFTR and for the suggestion!

For #137579 - I think there is still unexpected behavior when we use the virtual index without OID cast - I'd assume this should return 0 rows:

SELECT * FROM crdb_internal.create_type_statements WHERE descriptor_id = (SELECT 'db.public.enum'::REGTYPE::INT - 100000);
--          100 | defaultdb     | public      |           108 | enum            | CREATE TYPE public.enum AS ENUM ('x', 'y', 'z') | {x,y,z}

But I'm not sure if it's important and whether we should do anything about it. I'm no longer blocked on that issue, and if you think we wouldn't want to change the behavior, I'm ok closing it.

bors r+

@craig craig bot merged commit 88b6c94 into cockroachdb:master Dec 18, 2024
22 checks passed
@yuzefovich yuzefovich deleted the udt-internal branch December 18, 2024 20:37
@michae2
Copy link
Collaborator

michae2 commented Dec 23, 2024

I thought after #125996 we were using two-part names in SHOW CREATE output. This PR changes the output of SHOW CREATE ALL TYPES to include database name. It seems like this might break some external tools, e.g. as described in #125827.

@yuzefovich
Copy link
Member Author

Hm, yeah, that's true - we now will see the DB name in the output of SHOW CREATE ALL TYPES. Looks like we haven't documented this feature in detail (cockroachdb/docs#12187 is still open), but the syntax does show up in the diagram at https://www.cockroachlabs.com/docs/stable/show-create.

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.

4 participants