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: organize / track better which vtables are implemented #61801

Closed
RichardJCai opened this issue Mar 10, 2021 · 0 comments · Fixed by #63144
Closed

sql: organize / track better which vtables are implemented #61801

RichardJCai opened this issue Mar 10, 2021 · 0 comments · Fixed by #63144
Labels
A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.

Comments

@RichardJCai
Copy link
Contributor

RichardJCai commented Mar 10, 2021

Currently, we have an allTableNames field which we try to list all the name of all vtables. This is useful for providing error messages when a user tries to access a vtable that we know is unimplemented and have in the allTableNames list. The big issue with this is that it is all manual and does not correspond to our actual vtable definitions.

type virtualSchema struct {
	name           string
	allTableNames  map[string]struct{}
	tableDefs      map[descpb.ID]virtualSchemaDef
	tableValidator func(*descpb.TableDescriptor) error // optional
	// Some virtual tables can be used if there is no current database set; others can't.
	validWithNoDatabaseContext bool
	// Some virtual schemas (like pg_catalog) contain types that we can resolve.
	containsTypes bool
}

Adding a name is manual and prone to error.

Some options we have moving forward with this is to

  1. Remove the field (it is only used for error logging), we have a separate pg_catalog_expected_diffs.json file now which we can use to track.
  2. Make it correspond to the the actual tables / table ids somehow so it's less manual and we know if we implement it or not. Can also add tests to check the name against if the table desc is created.

In general, I think it would be nice to be able to track the status of vtables and see their implementations all in one place. Using a map that maps the table name to a struct holding it's definition, oid and status could be nice / an option?

We should choose one reliable source of truth on the status of different vtables.

@RichardJCai RichardJCai added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-sql-vtables Virtual tables - pg_catalog, information_schema etc labels Mar 10, 2021
@mnovelodou mnovelodou self-assigned this Mar 11, 2021
mnovelodou pushed a commit to mnovelodou/cockroach that referenced this issue Mar 16, 2021
virtual tables

Previously, Unimplemented virtual tables where tables
listed in the virtual schema which do not have a definition
This was inadequate because:
- A lot of tables where added for compatibility reasons
which are not longer considered umimplemented
- A tool hung because was expecting an unimplemented table
return rows
To address this, this patch:
- Adds a new field to the virtualSchemaTable to set a virtual
table as unimplemented
- Added a setting for enable / disable queries against
virtual tables

Release note (sql change): setting for enable/disable queries against
unimplemented virtual tables

Fixes cockroachdb#61866 cockroachdb#61801
mnovelodou pushed a commit to mnovelodou/cockroach that referenced this issue Apr 6, 2021
…ableNames

Previously, allTableNames have implemented and unimplemented tableNames
This was inadequate because the duplicity makes difficult to maintain
To address this, this patch refactors allTableNames into
unimplementedTableNames to keep only the tables that are not in tableDefs

Release note: None

Fixes cockroachdb#61801
mnovelodou pushed a commit to mnovelodou/cockroach that referenced this issue Apr 8, 2021
Previously, allTableNames have implemented and unimplemented tableNames
This was inadequate because the duplicity makes difficult to maintain
To address this, this patch refactors allTableNames into
undefinedTables to keep only the tables that are not in tableDefs

Release note: None

Fixes cockroachdb#61801
craig bot pushed a commit that referenced this issue Apr 12, 2021
63144: sql: refactoring virtualSchemaTable.allTableNames into undefinedTables r=RichardJCai a=mnovelodou

Previously, allTableNames have implemented and unimplemented tableNames
This was inadequate because the duplicity makes difficult to maintain
To address this, this patch refactors allTableNames into
undefinedTables to keep only the tables that are not in tableDefs

Release note: None

Fixes #61801

Co-authored-by: MiguelNovelo <[email protected]>
@craig craig bot closed this as completed in 345f9d7 Apr 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants