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.

Sign up for GitHub

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

Jump to bottom

sql: generate unique table IDs for virtual tables #32963

Closed
knz opened this issue Dec 9, 2018 · 0 comments · Fixed by #33697
Closed

sql: generate unique table IDs for virtual tables #32963

knz opened this issue Dec 9, 2018 · 0 comments · Fixed by #33697
Labels
A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue help wanted Help is requested / needed by the one who filed the issue to fix it.

Comments

@knz
Copy link
Contributor

knz commented Dec 9, 2018 •
edited
Loading

This is pursuant of #32442 towards #19472.

To attach comments to virtual tables they must have stable table IDs that can be used to populate system.comments.
However meanwhile we do not want them to be attached to range IDs because they are virtual.

Proposal:

  1. allocate IDs for vtables statically in the code. This ensures that IDs remain stable as the code evolves.

  2. use negative IDs, and have the negative value become an indication that the table is virtual (IsVirtualDesc).

  3. make COMMENT ON do the right thing for them

  4. make crdb_internal.tables list them alongside the others so that SHOW TABLES can list the comments for them.
@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-vtables Virtual tables - pg_catalog, information_schema etc labels Dec 9, 2018
@knz knz added E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue labels Dec 9, 2018
@knz knz mentioned this issue Dec 9, 2018
Closed
@knz knz added the help wanted Help is requested / needed by the one who filed the issue to fix it. label Dec 9, 2018
@hueypark hueypark mentioned this issue Jan 1, 2019
Closed
@hueypark hueypark mentioned this issue Jan 13, 2019
Merged
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 29, 2019
@RaduBerinde
opt: fix cached plan invalidation with some virtual tables
fb7c959 
It turns out that many virtual tables (e.g. in `pg_catalog` schema)
have instances for each catalog (and the context differs within each
catalog). Unfortunately, these instances all share the same virtual
table ID. This violates our assumptions and prevents us from
invalidating cached plans properly.

This change fixes this by also putting the database ID in the StableID
for virtual tables.

Note that the situation is in fact even worse: all virtual tables have
the *same* ID. This is being addressed separately (cockroachdb#33697, cockroachdb#32963).
Fortunately this doesn't currently cause problems in practice because
virtual tables have static names and can't be involved in FKs.

Release note: None
RaduBerinde added a commit to RaduBerinde/cockroach that referenced this issue Jan 29, 2019
@RaduBerinde
opt: fix cached plan invalidation with some virtual tables
519de24 
It turns out that many virtual tables (e.g. in `pg_catalog` schema)
have instances for each catalog (and the context differs within each
catalog). Unfortunately, these instances all share the same virtual
table ID. This violates our assumptions and prevents us from
invalidating cached plans properly.

This change fixes this by also putting the database ID in the StableID
for virtual tables.

Note that the situation is in fact even worse: all virtual tables have
the *same* ID. This is being addressed separately (cockroachdb#33697, cockroachdb#32963).
Fortunately this doesn't currently cause problems in practice because
virtual tables have static names and can't be involved in FKs.

Release note: None
craig bot pushed a commit that referenced this issue Feb 8, 2019
@hueypark
Merge #33697
adf3a1b 
33697: sql: make pg_catalog OIDs stable and refer to the descriptor IDs directly r=knz a=hueypark

Fixes #32940
Fixes #32963
Fixes #34710
Enables #32964

This patch changes the definition of vtables to assign a unique table
ID to every virtual table. Moreover, it also extends vtable
descriptors to assign proper column IDs to virtual table columns. This
aims to support the logical planning code, the table and plan caches,
and simplify+fix the introspection tables. Incidentally it also makes
it possible to use COMMENT ON on virtual tables and their columns too.

The table IDs are picked descending from MaxUint32 although this may
be refined in future PRs to accommodate the numbering of virtual
schema descriptors.

Incidentally `pg_catalog.pg_attribute` is fixed to properly use the
column ID in column `attnum`, so that `attnum` remains stable across
column drops.

Release note (sql change): Virtual tables in `pg_catalog`,
`information_schema` etc now support `COMMENT ON` like regular tables.

Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com>
@craig craig bot closed this as completed in #33697 Feb 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-sql-vtables Virtual tables - pg_catalog, information_schema etc C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) E-easy Easy issue to tackle, requires little or no CockroachDB experience good first issue help wanted Help is requested / needed by the one who filed the issue to fix it.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

sql: make pg_catalog OIDs stable and refer to the descriptor IDs directly hueypark/cockroach
1 participant
@knz