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: extend SHOW CREATE TABLE to use fully-qualified names #137719

Closed
wants to merge 1 commit into from

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Dec 18, 2024

This commit extends SHOW CREATE TABLE syntax to use fully-qualified names for tables as well as user-defined types used in the table. This is needed to make the recreation of stmt bundles collected on multi-region tables easier. Note that for UDTs this behavior was already the case up until this Summer when we switched to not showing the DB name for portability. The stmt bundle recreation case needs the exact reproduction since we might be dealing with cross-DB references and things like crdb_internal_region type unavailable in the current DB when creating a multi-region table.

SHOW CREATE TABLE syntax is extended to support FULLY_QUALIFIED option. We already had a documented REDACT option, so any combination of these two options is now supported. However, at least as of right now, I chose to not document this new option. We now use the new option when building the stmt bundle.

It is powered by adding two new columns create_statement_fq and create_redactable_fq to crdb_internal.create_statements virtual table. These columns are only populated for tables and will remain empty strings for views and sequences. The delegated show query is adjusted to use the proper column based on the options.

Informs: #99131.
Fixes: #123897.
Epic: None

Release note: None

This commit extends SHOW CREATE TABLE syntax to use fully-qualified
names for tables as well as user-defined types used in the table. This
is needed to make the recreation of stmt bundles collected on
multi-region tables easier. Note that for UDTs this behavior was already
the case up until this Summer when we switched to not showing the DB
name for portability. The stmt bundle recreation case needs the exact
reproduction since we might be dealing with cross-DB references and
things like `crdb_internal_region` type unavailable in the current DB
when creating a multi-region table.

`SHOW CREATE TABLE` syntax is extended to support `FULLY_QUALIFIED`
option. We already had a documented `REDACT` option, so any combination
of these two options is now supported. However, at least as of right
now, I chose to not document this new option. We now use the new option
when building the stmt bundle.

It is powered by adding two new columns `create_statement_fq` and
`create_redactable_fq` to `crdb_internal.create_statements` virtual
table. These columns are only populated for tables and will remain empty
strings for views and sequences. The delegated show query is adjusted to
use the proper column based on the options.

Release note: None
@yuzefovich yuzefovich requested review from a team as code owners December 18, 2024 22:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

I think it would be a lot simpler to add a USE statement to bundles. If we go down the route of making CREATE statements fully-qualified, wouldn't we have to add that ability to every type of CREATE statement that could appear in schema.sql?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @spilchen)

@yuzefovich
Copy link
Member Author

yuzefovich commented Dec 23, 2024

We already use some CREATE statements that use fully-qualified names for bundles (and will use more with #137662), and I think we should embrace this approach fully.

I considered using USE statement, but it seemed more error-prone. Consider the following example: we have two MR-enabled databases with a RBR table in each; the schema of RBR table has invisible column of crdb_internal_region type that right now would only have schema in the type name, so we would need to have a sequence like:

CREATE DATABASE mr1 ...;
CREATE DATABASE mr2 ...;
...
USE mr1;
CREATE TABLE t1 ...;
...
USE mr2;
CREATE TABLE t2 ...;

Now let's add user-defined types into the mix. Due to how we populate schema.sql, we would first handle all UDTs, and only then we'd add all tables, so now the file becomes something like:

CREATE DATABASE mr1 ...;
CREATE DATABASE mr2 ...;
...
USE mr1;
CREATE TYPE udt1 ...;
...
USE mr2;
CREATE TYPE udt2 ...;
...
USE mr1;
CREATE TABLE t1 ... <using udt1> ...;
...
USE mr2;
CREATE TABLE t2 ... <using udt2> ...;

Effectively we need to teach the bundle building code to be DB-aware for all objects. I actually started going down this path a bit, but then UDTs needed the special handling in addition to tables, and I thought that adjusting the CREATE stmts themselves would be cleaner (so I added #137580 for UDTs).

The alternative approach (which is what this PR helps with) is to ensure that all CREATE statements included in schema.sql use fully-qualified naming, but we already have that available for some (via crdb_internal.create_*_statements vtables) and it doesn't seem difficult to add that if necessary (see #137580). We use delegated SHOW CREATE statements for tables, sequences, and views that is powered by a more involved query to additionally pull in zone configs, so this PR makes the change to SHOW CREATE TABLE syntax. After this change I think that bundle recreation with cross-db views and sequences still might not work, and we still might need to ensure that fully-qualified naming is used for UDFs and procedures.


Having typed this all out, I don't feel strongly about USE approach or this fully-qualified naming in CREATE approach.

@yuzefovich
Copy link
Member Author

I implemented the USE approach in #138023, and it seems cleaner, so let's hold off on this change.

@yuzefovich
Copy link
Member Author

We implemented the USE approach in #138023, so this is no longer needed.

@yuzefovich yuzefovich closed this Jan 8, 2025
@yuzefovich yuzefovich deleted the bundle-fq branch January 8, 2025 01:53
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.

bundle: recreating a bundle with UDTs in a non-default database doesn't work
3 participants