Skip to content

sql: implement new crdb_internal virtual tables to inspect descriptors#17422

Merged
knz merged 2 commits intocockroachdb:masterfrom
knz:20170803-desc-deps
Aug 4, 2017
Merged

sql: implement new crdb_internal virtual tables to inspect descriptors#17422
knz merged 2 commits intocockroachdb:masterfrom
knz:20170803-desc-deps

Conversation

@knz
Copy link
Copy Markdown
Contributor

@knz knz commented Aug 3, 2017

The views in pg_catalog and information_schema provide introspection
over the logical schema (pure SQL view) however when testing/debugging
it is also important to see the descriptor IDs and also other fields
of descriptors.

This patch provides help for debugging with the following new virtual
tables:

  • crdb_internal.table_columns: detailed column IDs and properties
  • crdb_internal.table_indexes: detailed index IDs and index properties
  • crdb_internal.index_columns: what columns are indexed and how
  • crdb_internal.backward_dependencies: which other descriptors each descriptor depends on
  • crdb_internal.forward_dependencies: which other descriptors each descriptor is depended on by

These virtual tables are intended to display the contents of
descriptors without any data transformation, so as to reveal any error
in the descriptors, if any.

Debugging/fsck tools can later use these to check schema validity.

Needed to test #17310.

cc @cockroachdb/sql

@knz knz requested review from RaduBerinde and vivekmenezes August 3, 2017 16:52
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented Aug 3, 2017

This change is Reviewable

@knz
sql: implement new crdb_internal virtual tables to inspect descriptors
e3af3f3 
The views in pg_catalog and information_schema provide introspection
over the logical schema (pure SQL view) however when testing/debugging
it is also important to see the descriptor IDs and also other fields
of descriptors.

This patch provides help for debugging with the following new virtual
tables:

- `crdb_internal.table_columns`: detailed column IDs and properties
- `crdb_internal.table_indexes`: detailed index IDs and index properties
- `crdb_internal.index_columns`: what columns are indexed and how
- `crdb_internal.backward_dependencies`: which other descriptors each descriptor depends on
- `crdb_internal.forward_dependencies`: which other descriptors each descriptor is depended on by

These virtual tables are intended to display the contents of
descriptors without any data transformation, so as to reveal any error
in the descriptors, if any.

Debugging/fsck tools can later use these to check schema validity.
@knz knz force-pushed the 20170803-desc-deps branch from fd11ac8 to 15bbf1e Compare August 3, 2017 17:07
@knz knz mentioned this pull request Aug 3, 2017
Merged
@knz knz force-pushed the 20170803-desc-deps branch 2 times, most recently from cc6b153 to 1c2919a Compare August 3, 2017 17:40
@vivekmenezes
Copy link
Copy Markdown
Contributor

vivekmenezes commented Aug 4, 2017

:lgtm:

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

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

		crdbInternalCreateStmtsTable,
		crdbInternalTableColumnsTable,
		crdbInternalTableIndicesTable,

s/Indices/Indexes

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

						colDir := parser.DNull
						if i >= len(idx.ColumnNames) {
							log.Errorf(ctx, "index descriptor for [%d@%d] (%s.%s@%s) has more key column IDs (%d) than names (%d) (corrupted schema?)",

It's a pity we log an error here rather than return an error to the user

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

						}
						if i >= len(idx.ColumnDirections) {
							log.Errorf(ctx, "index descriptor for [%d@%d] (%s.%s@%s) has more key column IDs (%d) than directions (%d) (corrupted schema?)",

same

Comments from Reviewable

@knz
sql: remove the dependencies column from crdb_internal.create_statements
148eeaa 
... because it was incomplete (missing FK relationships).

The two new tables `crdb_internal.forward_dependencies` and
`crdb_internal.backward_dependencies` can be used instead.
@knz knz force-pushed the 20170803-desc-deps branch from 1c2919a to 148eeaa Compare August 4, 2017 12:30
@knz
Copy link
Copy Markdown
Contributor Author

knz commented Aug 4, 2017

TFYR!

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions.

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

Previously, vivekmenezes wrote…

s/Indices/Indexes

Done.

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

Previously, vivekmenezes wrote…

It's a pity we log an error here rather than return an error to the user

Added a comment to clarify.

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

Previously, vivekmenezes wrote…

same

ditto

Comments from Reviewable

@knz knz merged commit 269b149 into cockroachdb:master Aug 4, 2017
@knz knz deleted the 20170803-desc-deps branch August 4, 2017 12:52
@RaduBerinde
Copy link
Copy Markdown
Member

RaduBerinde commented Aug 4, 2017

Nice!

@andreimatei
Copy link
Copy Markdown
Contributor

andreimatei commented Aug 7, 2017

Is there also a virtual table exposing the descriptors themselves (as serialized protos)? Cause that's what got me all excited.
And if there is, could we please use it here:

cockroach/pkg/sql/lease.go

Line 421 in e554249

txn.SetFixedTimestamp(prevTimestamp)

(it'd need to support the historical read sql syntax, but hopefully that'd happen automatically)

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.

Comments from Reviewable

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Aug 7, 2017

@andreimatei what are you talking about? We do not need a virtual table; system.descriptor already does exactly that.

@andreimatei
Copy link
Copy Markdown
Contributor

andreimatei commented Aug 7, 2017

Oh I didn't know about system.descriptor, cool. But then do we really need all these new tables? Can't most of them just be functions operating on the descriptors from system.descriptor?

Also @vivekmenezes since this system.descriptor exists, I think we should use it extensively, for example here:

cockroach/pkg/sql/lease.go

Line 421 in e554249

txn.SetFixedTimestamp(prevTimestamp)

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Aug 7, 2017

The logic currently implemented by the new tables would be very cumbersome to express in pure SQL. Adding a bunch of built-in functions to decode the descriptors and provide this logic would work too, however I figured that a virtual tables provides a more natural approach to inspect this type of data.

@knz
Copy link
Copy Markdown
Contributor Author

knz commented Aug 7, 2017

Note for example that these vtables can be subject of joins, whereas built-in functions that extract arrays from descriptors would not be easily usable in joins.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vivekmenezes vivekmenezes Awaiting requested review from vivekmenezes

@RaduBerinde RaduBerinde Awaiting requested review from RaduBerinde

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@knz @cockroach-teamcity @vivekmenezes @RaduBerinde @andreimatei