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: implement new crdb_internal virtual tables to inspect descriptors #17422

Merged
merged 2 commits into from
Aug 4, 2017

Conversation

knz
Copy link
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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

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 2 times, most recently from cc6b153 to 1c2919a Compare August 3, 2017 17:40
@vivekmenezes
Copy link
Contributor

: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

... 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
Copy link
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
Member

Nice!

@andreimatei
Copy link
Contributor

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:

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
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
Contributor

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:

txn.SetFixedTimestamp(prevTimestamp)

@knz
Copy link
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants