sql: add SHOW CREATE TABLES / SHOW CREATE ALL TABLES commands #59412

RichardJCai · 2021-01-25T23:46:02Z

sql: add SHOW CREATE TABLES / SHOW CREATE ALL TABLES commands

Release note (sql change): Add support for SHOW CREATE TABLES [table_list]
and SHOW CREATE ALL TABLES commands.
SHOW CREATE ALL TABLES and SHOW CREATE TABLES only list the tables in the
current database.

Both commands output the schema_name, table_name, and
create_statement for a set of tables.

Example usage:

  > SHOW CREATE TABLES t, t2;
   schema_name | table_name |                create_statement
  -------------+------------+--------------------------------------------------
   public      | t          | CREATE TABLE public.t (FAMILY "primary" (rowid)
               |            | );
   public      | t2         | CREATE TABLE public.t2 (
               |            |     x INT8 NULL,
               |            |     y STRING NULL,
               |            |     FAMILY "primary" (x, y, rowid)
               |            | );
  (2 rows)

cockroach-teamcity · 2021-01-25T23:46:09Z

This change is

rafiss

thanks for the quick turn around!

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

pkg/sql/delegate/show_table.go, line 230 at r1 (raw file):

    ) AS create_statement
FROM
    crdb_internal.create_statements

i think we'd only want to include the results from the current database.

maybe if the current database is "", we could include them all? the old dump command had a --dump-all that i think we'd want to offer a workaround for.

pkg/sql/delegate/show_table.go, line 278 at r1 (raw file):

//   %[1]s the given table names as SQL string literal.
//   %[2]s the table IDs.
func (d *delegator) showTablesDetails(names tree.TableNames, query string) (tree.Statement, error) {

i feel like there should be a good way to reuse the same function for the old SHOW CREATE TABLE x and SHOW CREATE TABLES x y z. what do you think?

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 2 at r1 (raw file):

# All querys are separated into SELECT create_statement
# and SELECT database_name, schema_name, table_name for formatting purposes.

should the old SHOW CREATE TABLE x command also include the new columns too? it seems like it could be confusing for them to differ

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 35 at r1 (raw file):

   parent_schema_id INT8 NOT NULL,
   locality STRING NULL
)

we toyed with the idea of adding a semicolon to the output here so that tools could more easily consume the output. i think we should do that, and update the old SHOW CREATE stmts too

pkg/sql/parser/sql.y, line 5051 at r1 (raw file):

// %Help: SHOW CREATE - display the CREATE statement for a table, sequence or view
// %Category: DDL
// %Text: SHOW CREATE [ TABLE | SEQUENCE | VIEW ] <tablename>

i think we'd need to update some of the help text here

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 2 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

should the old SHOW CREATE TABLE x command also include the new columns too? it seems like it could be confusing for them to differ

I wasn't sure if it was okay to change the column set but right now I don't think the first column for SHOW CREATE TABLE x just returns x anyway so I'm not sure how useful that is.

I think it should be okay to change, thoughts?

RichardJCai

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

pkg/sql/delegate/show_table.go, line 230 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we'd only want to include the results from the current database.

maybe if the current database is "", we could include them all? the old dump command had a --dump-all that i think we'd want to offer a workaround for.

Done, I think crdb_internal.create_statements defaults to the current database regardless.

pkg/sql/delegate/show_table.go, line 278 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i feel like there should be a good way to reuse the same function for the old SHOW CREATE TABLE x and SHOW CREATE TABLES x y z. what do you think?

Refactored the function so the logic could be shared.

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 2 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I wasn't sure if it was okay to change the column set but right now I don't think the first column for SHOW CREATE TABLE x just returns x anyway so I'm not sure how useful that is.

I think it should be okay to change, thoughts?

Followup: same as the semi colon one, can we split that into a second issue, easier to make a PR purely for that.

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 35 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we toyed with the idea of adding a semicolon to the output here so that tools could more easily consume the output. i think we should do that, and update the old SHOW CREATE stmts too

Added a semi colon here, can we update the old SHOW CREATE stmts in a separate issue? There's a lot of logic tests using them, I don't want to balloon this PR, would rather have a separate one for that.

pkg/sql/parser/sql.y, line 5051 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think we'd need to update some of the help text here

Done.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/ccl/logictestccl/testdata/logic_test/zone, line 662 at r2 (raw file):

  PARTITION p1 VALUES IN ((1)),
  PARTITION p2 VALUES IN ((2))
);;

interesting, there's a double semicolon here..

pkg/sql/delegate/show_table.go, line 230 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Done, I think crdb_internal.create_statements defaults to the current database regardless.

yeah it does, but if you are running through the internal executor, then the current database is "", and using "".crdb_internal.create_statements would show info from all databases

pkg/sql/delegate/show_table.go, line 53 at r2 (raw file):

    1, 2;`

	return d.populateQueryDetails([]*tree.UnresolvedObjectName{n.Name}, showCreateQuery, false)

we should document inline constants like this:

... , showCreateQuery, false /* onlyQueryCurrentDB */)

but also i'm a bit confused -- how does this work if you're allowed to lookup this table name in other databases?

pkg/sql/delegate/show_table.go, line 109 at r2 (raw file):

	if n.WithComment {
		getColumnsQuery += `,
    col_description(%[6]s, attnum) AS comment`

wouldn't we want this to be a numeric value still?

pkg/sql/delegate/show_table.go, line 139 at r2 (raw file):

    LEFT OUTER JOIN pg_attribute
        ON column_name = pg_attribute.attname
        AND attrelid = %[6]s`

also should be numeric still?

pkg/sql/delegate/show_table.go, line 252 at r2 (raw file):

//   %[1]s the database name as SQL string literal.
//   %[2]s the unqualified table name(s) as SQL string literal.
//   %[3]s the given table name as SQL string literal. Remove.

is "Remove" an old comment?

pkg/sql/delegate/show_table.go, line 254 at r2 (raw file):

//   %[3]s the given table name as SQL string literal. Remove.
//   %[4]s the database name as SQL identifier.
//   %[5]s the schema name as SQL string literal. Not used for SHOW TABLES.

should it say "Not used for SHOW CREATE TABLES"? why is it not used?

pkg/sql/delegate/show_table.go, line 295 at r2 (raw file):

		databaseLiteral = d.evalCtx.SessionData.Database
		// DatabaseIdentifier is used in the query, double quote are needed
		// in the case where the database name includes spaces.

it's also needed if the name is mixed-case

pkg/sql/delegate/show_table.go, line 296 at r2 (raw file):

		// DatabaseIdentifier is used in the query, double quote are needed
		// in the case where the database name includes spaces.
		databaseIdentifier = strconv.Quote(databaseLiteral)

i don't think strconv is fully robust... check out lexbase.EncodeEscapedSQLIdent

pkg/sql/delegate/show_table.go, line 306 at r2 (raw file):

		givenTableName = resNames[0].String()

		if !onlyQueryCurrentDB {

i think this should would make more sense as an "else" condition right after line 296. why do it here?

so is the idea here that you can't do SHOW CREATE TABLES database.schema.table?

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 2 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Followup: same as the semi colon one, can we split that into a second issue, easier to make a PR purely for that.

ok yeah, can you file the issue? IMO we shouldn't release 21.1 with these differing. it would be a weird user experience

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 35 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Added a semi colon here, can we update the old SHOW CREATE stmts in a separate issue? There's a lot of logic tests using them, I don't want to balloon this PR, would rather have a separate one for that.

sounds good, can you file the issue? (you can use the same the same issue as above)

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/ccl/logictestccl/testdata/logic_test/zone, line 662 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

interesting, there's a double semicolon here..

Oops, yeah the logic for adding ZONECONFIG adds a semi colon, made sense when it previously didn't have a semicolon

pkg/sql/delegate/show_table.go, line 230 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

yeah it does, but if you are running through the internal executor, then the current database is "", and using "".crdb_internal.create_statements would show info from all databases

Oh fair, makes sense to explicitly specify it here then.

pkg/sql/delegate/show_table.go, line 53 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we should document inline constants like this:
... , showCreateQuery, false /* onlyQueryCurrentDB */)
but also i'm a bit confused -- how does this work if you're allowed to lookup this table name in other databases?

Searching up the table only applies for the existing SHOW TABLE commands, it's possible to look up tables in other databases by using the fully qualified name.

pkg/sql/delegate/show_table.go, line 139 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

also should be numeric still?

I changed it so the number is converted to a string in populateQueryDetails.

The reason I changed it to string is because when we have multiple ids, we convert the ids to a string in the format "1,2,3" and to consolidate the logic, even if there is only one id, it gets converted string still.

pkg/sql/delegate/show_table.go, line 296 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i don't think strconv is fully robust... check out lexbase.EncodeEscapedSQLIdent

Ah thanks good call

pkg/sql/delegate/show_table.go, line 306 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think this should would make more sense as an "else" condition right after line 296. why do it here?

so is the idea here that you can't do SHOW CREATE TABLES database.schema.table?

Right now, that's valid syntax but searching up tables in other databases won't appear.

Do you think we should make this explicitly invalid syntax?

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 2 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

ok yeah, can you file the issue? IMO we shouldn't release 21.1 with these differing. it would be a weird user experience

yep I'll make sure to get that in with this

RichardJCai

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/ccl/logictestccl/testdata/logic_test/zone, line 662 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Oops, yeah the logic for adding ZONECONFIG adds a semi colon, made sense when it previously didn't have a semicolon

Done.

pkg/sql/delegate/show_table.go, line 53 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Searching up the table only applies for the existing SHOW TABLE commands, it's possible to look up tables in other databases by using the fully qualified name.

Done.

pkg/sql/delegate/show_table.go, line 254 at r2 (raw file):

yeah it does, but if you are running through the internal executor, then the current database is "", and using "".crdb_internal.create_statements would show info from all databases
21 hours ago
RichardJCaiRichard Cai

Removed

pkg/sql/delegate/show_table.go, line 306 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Right now, that's valid syntax but searching up tables in other databases won't appear.

Do you think we should make this explicitly invalid syntax?

Also the reason it isn't an else condition is because it's only applicable to SHOW CREATE TABLE commands where theres one table.

This code is a little messy because SHOW CREATE TABLE and SHOW CREATE TABLES differ in which databases can be searched. SHOW CREATE TABLE allows us to query any table in any db whereas SHOW CREATE TABLES currently only supports the current db.

Do you think it would actually be better to split the functions into two again since the underlying logic is actually different enough, it lets us cleanup the code a bit, we can extract the table resolution part thats similar.

pkg/sql/logictest/testdata/logic_test/show_create_tables, line 35 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

sounds good, can you file the issue? (you can use the same the same issue as above)

Created issue here: #59772

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/sql/delegate/show_table.go, line 306 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Also the reason it isn't an else condition is because it's only applicable to SHOW CREATE TABLE commands where theres one table.

This code is a little messy because SHOW CREATE TABLE and SHOW CREATE TABLES differ in which databases can be searched. SHOW CREATE TABLE allows us to query any table in any db whereas SHOW CREATE TABLES currently only supports the current db.

Do you think it would actually be better to split the functions into two again since the underlying logic is actually different enough, it lets us cleanup the code a bit, we can extract the table resolution part thats similar.

yeah i agree, the only part that should be extracted is the part where there's common logic

rafiss

overall lgtm! but leaving a suggestion to consider..

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

pkg/ccl/logictestccl/testdata/logic_test/zone, line 662 at r3 (raw file):

  PARTITION p1 VALUES IN ((1)),
  PARTITION p2 VALUES IN ((2))
);;

does this test need to be updated now?

pkg/sql/delegate/show_table.go, line 320 at r3 (raw file):

	switch {
	case len(names) == 1:

if you feel like the code would be cleaner as two functions, then let's go with that. it's not a good abstraction for a function to have an if in the middle that causes divergent behavior in the two conditional branches. only the logic that actually is the same between the two cases should be the part thats extracted as a function.

rafiss

thanks for this!

Reviewed 5 of 8 files at r1, 1 of 4 files at r2, 1 of 2 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @RichardJCai)

RichardJCai · 2021-02-12T18:24:59Z

Removing SCHEMA column, realized schemas are always explicitly added to the create statement output anyway.

Release note (sql change): Add support for SHOW CREATE TABLE [table_list] and SHOW CREATE ALL TABLES commands. SHOW CREATE ALL TABLES and SHOW CREATE TABLE only list the tables in the current database. Note: previously SOHW CREATE TABLE was possible with a single table but now we allow multiple tables as an argument. Both commands output the table_name, and create_statement for a set of tables. Example usage: > SHOW CREATE TABLE t, t2; table_name | create_statement ------------+-------------------------------------------------- t | CREATE TABLE public.t (FAMILY "primary" (rowid) | ); t2 | CREATE TABLE public.t2 ( | x INT8 NULL, | y STRING NULL, | FAMILY "primary" (x, y, rowid) | ); (2 rows)

RichardJCai · 2021-02-12T20:03:35Z

I realized the logic for SHOW CREATE TABLE with multiple tables is wrong in this PR when it comes to multiple zone configs.

Closing this PR in favor for just adding the alias to the crdb_internal.show_create_all_tables builtin.

Will revisit adding SHOW CREATE TABLE for multiple tables if we see value to it in the future, I believe the original usecase for this ticket from #53488 will be covered by the SHOW CREATE ALL TABLES command regardless.

RichardJCai · 2021-02-12T20:11:12Z

Opened #60539 to handle just the SHOW CREATE ALL TABLES case

RichardJCai requested review from rafiss and a team January 25, 2021 23:46

RichardJCai force-pushed the show_create_tables branch from 533e320 to 8e6edca Compare January 26, 2021 15:59

rafiss requested changes Jan 27, 2021

View reviewed changes

RichardJCai commented Jan 27, 2021

View reviewed changes

RichardJCai force-pushed the show_create_tables branch from 8e6edca to 74f251c Compare January 29, 2021 21:16

RichardJCai commented Jan 29, 2021

View reviewed changes

RichardJCai requested a review from rafiss January 29, 2021 21:17

RichardJCai force-pushed the show_create_tables branch 2 times, most recently from 3ee186c to b5560ba Compare February 1, 2021 16:51

rafiss requested changes Feb 2, 2021

View reviewed changes

RichardJCai commented Feb 2, 2021

View reviewed changes

RichardJCai mentioned this pull request Feb 3, 2021

sql: update show create tables command to show schema_name and add semi colons #59772

Closed

RichardJCai commented Feb 3, 2021

View reviewed changes

RichardJCai force-pushed the show_create_tables branch from b5560ba to f00c867 Compare February 3, 2021 19:51

rafiss requested changes Feb 3, 2021

View reviewed changes

rafiss approved these changes Feb 3, 2021

View reviewed changes

RichardJCai force-pushed the show_create_tables branch from f00c867 to c5f1fad Compare February 3, 2021 20:24

rafiss approved these changes Feb 3, 2021

View reviewed changes

RichardJCai force-pushed the show_create_tables branch from c5f1fad to cb3546a Compare February 3, 2021 22:29

RichardJCai force-pushed the show_create_tables branch from cb3546a to 1eb6ac8 Compare February 12, 2021 17:00

RichardJCai requested a review from a team as a code owner February 12, 2021 17:00

RichardJCai force-pushed the show_create_tables branch from 1eb6ac8 to bc08ef6 Compare February 12, 2021 18:10

RichardJCai force-pushed the show_create_tables branch from bc08ef6 to 893fded Compare February 12, 2021 18:26

RichardJCai closed this Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add SHOW CREATE TABLES / SHOW CREATE ALL TABLES commands #59412

sql: add SHOW CREATE TABLES / SHOW CREATE ALL TABLES commands #59412

RichardJCai commented Jan 25, 2021 •

edited

Loading

cockroach-teamcity commented Jan 25, 2021

rafiss left a comment

RichardJCai left a comment

RichardJCai left a comment

rafiss left a comment

RichardJCai left a comment

RichardJCai left a comment

RichardJCai left a comment

rafiss left a comment

rafiss left a comment

rafiss left a comment

RichardJCai commented Feb 12, 2021

RichardJCai commented Feb 12, 2021

RichardJCai commented Feb 12, 2021

sql: add SHOW CREATE TABLES / SHOW CREATE ALL TABLES commands #59412

sql: add SHOW CREATE TABLES / SHOW CREATE ALL TABLES commands #59412

Conversation

RichardJCai commented Jan 25, 2021 • edited Loading

cockroach-teamcity commented Jan 25, 2021

rafiss left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

RichardJCai left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

RichardJCai commented Feb 12, 2021

RichardJCai commented Feb 12, 2021

RichardJCai commented Feb 12, 2021

RichardJCai commented Jan 25, 2021 •

edited

Loading