sql: provide table comments for virtual tables #34764

hueypark · 2019-02-10T07:56:37Z

Release note (sql change): CockroachDB now provides usable comments
with optional documentation URLs for the virtual tables in
pg_catalog, information_schema and crdb_internal. Use SHOW TABLES [FROM ...] WITH COMMENT to read. Note that crdb_internal
tables remain an experimental feature subject to change without
notice.

cockroach-teamcity · 2019-02-10T07:56:43Z

This change is

knz · 2019-02-25T17:50:42Z

pkg/sql/crdb_internal.go

+			e := vEntries[virtSchemaName]
+			for _, tName := range e.orderedDefNames {
+				table := e.defs[tName].desc
+				if err := addRow(


make a single function that takes the desc as argument and does the common thing between this and below

knz · 2019-02-25T17:51:41Z

pkg/sql/crdb_internal.go

@@ -644,6 +673,7 @@ CREATE TABLE crdb_internal.session_trace (
  message     STRING NOT NULL,     -- The logged message.
  age         INTERVAL NOT NULL    -- The age of this message relative to the beginning of the trace.
 )`,
+	comment: "latest trace collected on this session (via SET TRACING={ON/OFF}).",


nit/style: when we opt out of full sentences (no capital at start) we also opt out of the final period. (Same as cluster settings)

Conversely you could opt into the capitals.

knz · 2019-02-25T17:52:12Z

pkg/sql/crdb_internal.go

@@ -736,7 +768,8 @@ func (p *planner) makeSessionsRequest(ctx context.Context) serverpb.ListSessions
 // crdbInternalLocalQueriesTable exposes the list of running queries
 // on the current node. The results are dependent on the current user.
 var crdbInternalLocalQueriesTable = virtualSchemaTable{
-	schema: fmt.Sprintf(queriesSchemaPattern, "node_queries"),
+	schema:  fmt.Sprintf(queriesSchemaPattern, "node_queries"),
+	comment: "the list of running queries on the current node. The results are dependent on the current user.",


inconsistent articles (either use "the" or not at start, but do the same consistently)

knz · 2019-02-25T17:52:49Z

pkg/sql/crdb_internal.go

@@ -960,7 +996,7 @@ var crdbInternalLocalMetricsTable = virtualSchemaTable{
  name               STRING NOT NULL,  -- name of the metric
  value							 FLOAT NOT NULL    -- value of the metric
 )`,
-
+	comment: "snapshot of the metrics on the current node.",


not sure "snapshot" is necessary. I think it's implied for all the tables.

knz · 2019-02-25T17:53:22Z

pkg/sql/crdb_internal.go

@@ -1038,6 +1075,7 @@ CREATE TABLE crdb_internal.create_statements (
  validate_statements STRING[] NOT NULL
 )
 `,
+	comment: "exposes the CREATE TABLE/CREATE VIEW statements.",


"the CREATE statements". No need to be specific, also your current phrasing would already be inaccurate.

hueypark

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

pkg/sql/crdb_internal.go, line 209 at r1 (raw file):

Previously, knz (kena) wrote…

make a single function that takes the desc as argument and does the common thing between this and below

Done.

pkg/sql/crdb_internal.go, line 676 at r1 (raw file):

nit/style: when we opt out of full sentences (no capital at start) we also opt out of the final period. (Same as cluster settings)
I choose to opt out of the final period.

pkg/sql/crdb_internal.go, line 772 at r1 (raw file):

Previously, knz (kena) wrote…

inconsistent articles (either use "the" or not at start, but do the same consistently)

Done.

pkg/sql/crdb_internal.go, line 999 at r1 (raw file):

Previously, knz (kena) wrote…

not sure "snapshot" is necessary. I think it's implied for all the tables.

Done.

pkg/sql/crdb_internal.go, line 1078 at r1 (raw file):

"the CREATE statements". No need to be specific, also your current phrasing would already be inaccurate.
I deleted the unnecessary comment.

knz

Thank you for your changes. I have some more suggestions below.

Reviewed 2 of 8 files at r1, 6 of 6 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @hueypark)

pkg/server/admin.go, line 1494 at r2 (raw file):

		dbName := (*string)(row[2].(*tree.DString))

		if sqlbase.IsVirtualTable(sqlbase.ID(tableID)) {

Thanks for noticing this. However I think this change belongs to a different PR. Can you extract it from this PR and file a different PR for this? Thank you

pkg/sql/crdb_internal.go, line 209 at r1 (raw file):

Previously, hueypark (Jaewan Park) wrote…

Done.

This will allocate an array, expand it and discard it for every row. Maybe it's a bit memory-heavy and inefficient? Consider this instead:

createTableRow := func(table *sqlbase.TableDescriptor, dbName string) error {
   ...
       return addRow(...)
}

(i.e. use a closure)

then below

if err := createTableRow(table, dbName); err != nil {
   return err
}

pkg/sql/crdb_internal.go, line 676 at r1 (raw file):

Previously, hueypark (Jaewan Park) wrote…

nit/style: when we opt out of full sentences (no capital at start) we also opt out of the final period. (Same as cluster settings)
I choose to opt out of the final period.

I still see the period here :)

this is a period: .

for example "latest trace collected on this session." -> "latest trace collected on this session"

pkg/sql/show_tables.go, line 52 at r2 (raw file):

SELECT
	i.table_name,
	obj_description(t.table_id::REGCLASS) AS comment

This seems to be better because it's shorter, however it's objectively worse for performance: the query optimizer cannot "See through" obj_description and see the join performed within, and cannot work to simplify the query. The result is that the comment table will be queried again for every table.

However I also understand that system.comment does not contain the virtual table comments.

Maybe the right way to do this is to add a new virtual table in information_schema (information_schema.comments?) that contains all the data from system.comments + the data in the .comment field for virtual tables.

This way you can preserve the join here, simply replacing system.comments by information_schema.comments.

hueypark

Thank you for your reviews. I changed the implementation based on your suggestion(add information_schema.comments).

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

pkg/sql/crdb_internal.go, line 209 at r1 (raw file):

Previously, knz (kena) wrote…

This will allocate an array, expand it and discard it for every row. Maybe it's a bit memory-heavy and inefficient? Consider this instead:
createTableRow := func(table *sqlbase.TableDescriptor, dbName string) error {
   ...
       return addRow(...)
}
(i.e. use a closure)

then below
if err := createTableRow(table, dbName); err != nil {
   return err
}

This function is now obsolete.

pkg/sql/crdb_internal.go, line 676 at r1 (raw file):

Previously, knz (kena) wrote…

I still see the period here :)

this is a period: .

for example "latest trace collected on this session." -> "latest trace collected on this session"

Done.

pkg/sql/show_tables.go, line 52 at r2 (raw file):

Previously, knz (kena) wrote…

This seems to be better because it's shorter, however it's objectively worse for performance: the query optimizer cannot "See through" obj_description and see the join performed within, and cannot work to simplify the query. The result is that the comment table will be queried again for every table.

However I also understand that system.comment does not contain the virtual table comments.

Maybe the right way to do this is to add a new virtual table in information_schema (information_schema.comments?) that contains all the data from system.comments + the data in the .comment field for virtual tables.

This way you can preserve the join here, simply replacing system.comments by information_schema.comments.

Done.
And I've added an id column to the information_schema.tables table to eliminate the unnecessary JOIN.

pkg/server/admin.go, line 1494 at r2 (raw file):

Previously, knz (kena) wrote…

Thanks for noticing this. However I think this change belongs to a different PR. Can you extract it from this PR and file a different PR for this? Thank you

Done.
But, This was necessary to pass the test If I do not use information_schema.comments approach.

This patch adds a `comment` to `virtualSchemaTable` and exposes it through a new virtual table `crdb_internal.predefined_comments`. This is then used by `pg_catalog.pg_description` to expose comments on virtual tables alongside those of real tables. This way `obj_description()` works on virtual tables too. Finally it modifies `SHOW TABLES WITH COMMENT` to use `pg_description` and expose the table comments. Release note (sql change): CockroachDB now provides usable comments with optional documentation URLs for the virtual tables in `pg_catalog`, `information_schema` and `crdb_internal`. Use `SHOW TABLES [FROM ...] WITH COMMENT` to read. Note that `crdb_internal` tables remain an experimental feature subject to change without notice.

knz · 2019-03-09T22:41:55Z

Thank you for your changes. All it all it was a good improvement.

I made a few last minute changes, including fixing col_description and obj_description. I also decided against adding table_id to information_schema.tables, because the risk of problem with 3rd party tools was too important. Instead I made show tables with comment use pg_description and aded crdb_internal.predefined_comments to support that.

Again thank you for your efforts!

bors r+

34764: sql: provide table comments for virtual tables r=knz a=hueypark Fixes #32964 Fixes #35581 Release note (sql change): CockroachDB now provides usable comments with optional documentation URLs for the virtual tables in `pg_catalog`, `information_schema` and `crdb_internal`. Use `SHOW TABLES [FROM ...] WITH COMMENT` to read. Note that `crdb_internal` tables remain an experimental feature subject to change without notice. Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com>

craig · 2019-03-09T23:23:50Z

Build succeeded

GitHub CI (Cockroach)

hueypark requested review from a team February 10, 2019 07:56

hueypark requested a review from a team as a code owner February 10, 2019 07:56

hueypark requested review from a team February 10, 2019 07:56

jordanlewis requested a review from knz February 25, 2019 17:37

knz reviewed Feb 25, 2019

View reviewed changes

hueypark force-pushed the provide-comment-crdb_internal branch from 8cde54f to 75ad40e Compare February 27, 2019 14:04

hueypark commented Feb 27, 2019

View reviewed changes

knz reviewed Feb 28, 2019

View reviewed changes

hueypark force-pushed the provide-comment-crdb_internal branch from 75ad40e to 7038a37 Compare March 3, 2019 17:00

hueypark requested a review from a team March 3, 2019 17:00

hueypark commented Mar 3, 2019

View reviewed changes

knz force-pushed the provide-comment-crdb_internal branch from 7038a37 to 6edb5d1 Compare March 9, 2019 18:32

knz changed the title ~~sql: provide table comments for crdb_internal~~ sql: provide table comments for virtual tables Mar 9, 2019

knz force-pushed the provide-comment-crdb_internal branch 2 times, most recently from dee5a08 to 3cf9062 Compare March 9, 2019 20:21

hueypark requested a review from a team March 9, 2019 20:21

knz force-pushed the provide-comment-crdb_internal branch from 3cf9062 to 0d7ee51 Compare March 9, 2019 21:39

knz force-pushed the provide-comment-crdb_internal branch from 0d7ee51 to 336a9fa Compare March 9, 2019 22:18

craig bot merged commit 336a9fa into cockroachdb:master Mar 9, 2019

rmloveland mentioned this pull request Mar 18, 2019

sql: provide table comments for virtual tables cockroachdb/docs#4524

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: provide table comments for virtual tables #34764

sql: provide table comments for virtual tables #34764

hueypark commented Feb 10, 2019 •

edited by knz

Loading

cockroach-teamcity commented Feb 10, 2019

knz Feb 25, 2019

knz Feb 25, 2019

knz Feb 25, 2019

knz Feb 25, 2019

knz Feb 25, 2019

hueypark left a comment

knz left a comment

hueypark left a comment •

edited

Loading

knz commented Mar 9, 2019

craig bot commented Mar 9, 2019

sql: provide table comments for virtual tables #34764

sql: provide table comments for virtual tables #34764

Conversation

hueypark commented Feb 10, 2019 • edited by knz Loading

cockroach-teamcity commented Feb 10, 2019

knz Feb 25, 2019

Choose a reason for hiding this comment

knz Feb 25, 2019

Choose a reason for hiding this comment

knz Feb 25, 2019

Choose a reason for hiding this comment

knz Feb 25, 2019

Choose a reason for hiding this comment

knz Feb 25, 2019

Choose a reason for hiding this comment

hueypark left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

hueypark left a comment • edited Loading

Choose a reason for hiding this comment

knz commented Mar 9, 2019

craig bot commented Mar 9, 2019

Build succeeded

hueypark commented Feb 10, 2019 •

edited by knz

Loading

hueypark left a comment •

edited

Loading