sql: add is_visible to indexes info table by wenyihu6 · Pull Request #84776 · cockroachdb/cockroach

wenyihu6 · 2022-07-21T01:06:09Z

This commit adds a new column is_visible to crdb_internal.table_indexes and
information_schema.statistics. It also adds a new column visible to the
output of following SQL statements:

SHOW INDEX FROM (table_name)
SHOW INDEXES FROM (table_name)
SHOW KEYS FROM (table_name)
SHOW INDEX FROM DATABASE (database_name)
SHOW INDEXES FROM DATABASE (database_name)
SHOW KEYS FROM DATABASE (database_name)

Since the not visible index feature has not been introduced yet, it is expected
for all test cases to output true for all is_visible or visible columns.

Assists: #72576, #84357

See also: #84783

Release note (sql change): A new column is_visible has been added to the table
crdb_internal.table_indexes and information_schema.statistics. A new column
visible has also been added to the output of SHOW INDEX, SHOW INDEXES, and
SHOW KEYS. The is_visible or visible column indicates whether the index is
visible to the optimizer.

cockroach-teamcity · 2022-07-21T01:06:17Z

This change is

wenyihu6 · 2022-07-21T01:07:55Z

The first commit comes from #84763. Update: the first commit has been merged into master. This branch is now rebased on master.

mgartner

Great job! Just a couple questions.

Reviewed 26 of 26 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @wenyihu6)

pkg/sql/delegate/show_database_indexes.go line 58 at r2 (raw file):

	getAllIndexesQuery += `
ORDER BY 1, 2, 4`

Can you explain why you added this?

pkg/sql/logictest/testdata/logic_test/storing line 14 at r2 (raw file):

SHOW INDEXES FROM t
----
table_name  index_name  non_unique  seq_in_index  column_name  direction  storing  implicit  visible

Do we need to add the visible column here? visible is an attribute of the entire index, not indexed columns. We don't include other attributes of indexes such as an inverted bool, a partial index predicate. Did we forget to add those or is there a reason we didn't? cc @cockroachdb/sql-experience

This PR adds a new field `NotVisible` to the struct `IndexDescriptor`. Since primary indexes cannot be not visible, it also includes another check in `pkg/sql/catalog/tabledesc/validate.go`. Since the invisible index feature has not been introduced yet, all indexes created should be visible. See also: cockroachdb#84776 Assists: cockroachdb#72576 Release note: none

wenyihu6 · 2022-07-21T20:42:18Z

pkg/sql/delegate/show_database_indexes.go line 58 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you explain why you added this?

I think this will make the result more deterministic. Having the result sorted by table_name, index_name, and seq_in_index will guarantee the order of the result.

michae2

Reviewed 26 of 26 files at r2, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @mgartner, @michae2, and @wenyihu6)

pkg/sql/delegate/show_database_indexes.go line 58 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you explain why you added this?

I asked Wenyi to add this (here). It makes the SHOW output deterministic, which makes it more compatible with MySQL / MariaDB SHOW output. Most of our other SHOW commands also have explicit ordering.

pkg/sql/logictest/testdata/logic_test/storing line 14 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Do we need to add the visible column here? visible is an attribute of the entire index, not indexed columns. We don't include other attributes of indexes such as an inverted bool, a partial index predicate. Did we forget to add those or is there a reason we didn't? cc @cockroachdb/sql-experience

We're trying to match the output of MySQL SHOW INDEXES.

This PR adds parsing support for `CREATE INDEX … NOT VISIBLE` and `CREATE TABLE (... INDEX() NOT VISIBLE)`. This PR does not add any logic to the optimizer, and executing it returns an “unimplemented” error immediately. Assists: cockroachdb#72576 See also: cockroachdb#84776 Release note (sql change): Parser now supports creating an index with the option to mark them as invisible. But no implementation has done yet, and executing it returns an “unimplemented” error immediately.

This commit adds a new field `NotVisible` to the struct `IndexDescriptor`. Since primary indexes cannot be not visible, it also includes another check in `pkg/sql/catalog/tabledesc/validate.go`. Since the invisible index feature has not been introduced yet, all indexes created should be visible. See also: cockroachdb#84776 Assists: cockroachdb#72576 Release note: none

84763: sql: add NotVisible to index descriptor r=wenyihu6 a=wenyihu6 This commit adds a new field `NotVisible` to the struct `IndexDescriptor`. Since primary indexes cannot be not visible, it also includes another check in `pkg/sql/catalog/tabledesc/validate.go`. Since the invisible index feature has not been introduced yet, all indexes created should be visible. See also: #84776 Assists: #72576 Release note: none Co-authored-by: wenyihu3 <wenyi.hu@cockroachlabs.com>

mgartner

Reviewed 19 of 26 files at r4, 32 of 32 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @michae2 and @wenyihu6)

pkg/sql/delegate/show_database_indexes.go line 58 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I asked Wenyi to add this (here). It makes the SHOW output deterministic, which makes it more compatible with MySQL / MariaDB SHOW output. Most of our other SHOW commands also have explicit ordering.

Thanks for the explanation!

pkg/sql/logictest/testdata/logic_test/storing line 14 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

We're trying to match the output of MySQL SHOW INDEXES.

Got it. Thanks!

This PR adds parsing support for `CREATE INDEX … NOT VISIBLE` and `CREATE TABLE (... INDEX() NOT VISIBLE)`. This PR does not add any logic to the optimizer, and executing it returns an “unimplemented” error immediately. Assists: cockroachdb#72576 See also: cockroachdb#84776 Release note (sql change): Parser now supports creating an index with the option to mark them as invisible. But no implementation has done yet, and executing it returns an “unimplemented” error immediately.

wenyihu6 · 2022-07-26T17:15:39Z

bors r+

craig · 2022-07-26T18:47:02Z

Build failed (retrying...):

GitHub CI (Cockroach)

michae2 · 2022-07-26T20:08:07Z

Let's try this again.

bors r+

craig · 2022-07-26T20:08:08Z

Already running a review

michae2 · 2022-07-26T20:08:23Z

bors r-

craig · 2022-07-26T20:08:25Z

Canceled.

michae2 · 2022-07-26T20:08:34Z

bors r+

craig · 2022-07-26T23:17:15Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-07-26T23:26:58Z

Build failed (retrying...):

GitHub CI (Cockroach)

This commit adds a new column `is_visible` to `crdb_internal.table_indexes` and `information_schema.statistics`. It also adds a new column `visible` to the output of following SQL statements: ``` SHOW INDEX FROM (table_name) SHOW INDEXES FROM (table_name) SHOW KEYS FROM (table_name) SHOW INDEX FROM DATABASE (database_name) SHOW INDEXES FROM DATABASE (database_name) SHOW KEYS FROM DATABASE (database_name) ``` Since the not visible index feature has not been introduced yet, it is expected for all test cases to output `true` for all `is_visible` or `visible` columns. Assists: cockroachdb#72576 See also: cockroachdb#84783 Release note (sql change): A new column `is_visible` has been added to the table `crdb_internal.table_indexes` and `information_schema.statistics`. A new column `visible` has also been added to the output of `SHOW INDEX`, `SHOW INDEXES`, and `SHOW KEYS`. The `is_visible` or `visible` column indicates whether the index is visible to the optimizer.

craig · 2022-07-26T23:29:10Z

Canceled.

wenyihu6 · 2022-07-27T10:18:20Z

bors r+

craig · 2022-07-27T11:42:41Z

Build succeeded:

GitHub CI (Cockroach)

wenyihu6 changed the title ~~1.5 add visible schema~~ sql: adds is_visible to indexes info table Jul 21, 2022

wenyihu6 force-pushed the 1.5-add-visible-schema branch from 076a1bb to 3eedca5 Compare July 21, 2022 01:10

wenyihu6 mentioned this pull request Jul 21, 2022

sql: add NotVisible to index descriptor #84763

Merged

wenyihu6 force-pushed the 1.5-add-visible-schema branch 2 times, most recently from b7a76c9 to 21c6230 Compare July 21, 2022 02:37

This was referenced Jul 21, 2022

sql: added CREATE … NOT VISIBLE to parser #83471

Closed

sql: add CREATE … NOT VISIBLE to parser #84783

Merged

sql: added Hidden to index descriptor and SHOW INDEX #83388

Closed

wenyihu6 marked this pull request as ready for review July 21, 2022 03:51

wenyihu6 requested a review from a team as a code owner July 21, 2022 03:51

wenyihu6 requested review from a team July 21, 2022 03:51

wenyihu6 requested review from a team as code owners July 21, 2022 03:51

wenyihu6 changed the title ~~sql: adds is_visible to indexes info table~~ sql: add is_visible to indexes info table Jul 21, 2022

wenyihu6 force-pushed the 1.5-add-visible-schema branch 2 times, most recently from 6300c57 to 7870847 Compare July 21, 2022 14:41

michae2 requested review from mgartner and michae2 July 21, 2022 17:07

mgartner reviewed Jul 21, 2022

View reviewed changes

wenyihu6 force-pushed the 1.5-add-visible-schema branch from 7870847 to 7e4fbc2 Compare July 21, 2022 20:43

michae2 approved these changes Jul 21, 2022

View reviewed changes

wenyihu6 mentioned this pull request Jul 22, 2022

sql: mismatch between MySQL version and CRDB in pg_metadata_test #84915

Closed

mgartner approved these changes Jul 22, 2022

View reviewed changes

wenyihu6 force-pushed the 1.5-add-visible-schema branch from b550060 to f1b31eb Compare July 22, 2022 23:14

wenyihu6 force-pushed the 1.5-add-visible-schema branch from f1b31eb to 56b25b5 Compare July 25, 2022 13:38

wenyihu6 force-pushed the 1.5-add-visible-schema branch 3 times, most recently from 5c18231 to e3832ef Compare July 26, 2022 14:49

michae2 mentioned this pull request Jul 26, 2022

sql: SHOW INDEXES output should be more stable #84357

Closed

wenyihu6 force-pushed the 1.5-add-visible-schema branch from e3832ef to 9e6ab9d Compare July 26, 2022 23:29

craig bot merged commit 4df883b into cockroachdb:master Jul 27, 2022

cockroach-teamcity mentioned this pull request Jul 27, 2022

sql: add is_visible to indexes info table cockroachdb/docs#14640

Closed

wenyihu6 deleted the 1.5-add-visible-schema branch September 1, 2022 20:47

Conversation

wenyihu6 commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jul 21, 2022

Uh oh!

wenyihu6 commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Jul 21, 2022

Uh oh!

michae2 left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

wenyihu6 commented Jul 26, 2022

Uh oh!

craig bot commented Jul 26, 2022

Uh oh!

michae2 commented Jul 26, 2022

Uh oh!

craig bot commented Jul 26, 2022

Uh oh!

michae2 commented Jul 26, 2022

Uh oh!

craig bot commented Jul 26, 2022

Uh oh!

michae2 commented Jul 26, 2022

Uh oh!

craig bot commented Jul 26, 2022

Uh oh!

craig bot commented Jul 26, 2022

Uh oh!

craig bot commented Jul 26, 2022

Uh oh!

wenyihu6 commented Jul 27, 2022

Uh oh!

craig bot commented Jul 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wenyihu6 commented Jul 21, 2022 •

edited

Loading

wenyihu6 commented Jul 21, 2022 •

edited

Loading