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: add proper support for hidden columns #26644

Closed
wants to merge 1 commit into from

Conversation

knz
Copy link
Contributor

@knz knz commented Jun 12, 2018

(Needed to implement pg_am and pg_opclass for #26504.)

Prior to this patch, CockroachDB already had full support for marking
arbitrary columns as "hidden" internally: a hidden column can be used
for queries but does not get automatically selected by a * in a
SELECT clause, and does not get automatically inserted/upserted into
when INSERT/UPSERT do not list any columns.

However, no control was given to users to determine which columns
would be hidden. The only column that could be hidden in practice was
the implicit rowid column created when CREATE does not specify a
primary key.

This patch complements the existing thorough support for hidden column
by properly exposing the "hidden" attribute through SQL:

  • any column can now be marked as hidden in CREATE using the column
    attribute NOT VISIBLE.
  • the attribute can now be changed with ALTER TABLE .. ALTER COLUMN .. SET [NOT] VISIBLE.

This change is further motivated by the need to annotate non-PK
columns in pg_catalog vtables, for compatibility with
PostgreSQL (this will be actually done in a later patch).

The output of SHOW CREATE now displays all columns with their
optional NOT VISIBLE attribute, instead of skipping over hidden
column as previously.

Note: the choice for the syntax "NOT VISIBLE" as opposed to "HIDDEN"
is mandated by the fact that the first word of an attribute must be a
reserved keyword, and adding reserved keywords is disruptive. This
way, the patch is properly backward-compatible.

Release note (sql change): clients can now set the visibility of a
column using the NOT VISIBLE attribute in CREATE TABLE or SET VISIBLE / SET NOT VISIBLE in ALTER TABLE .. ALTER COLUMN.

Prior to this patch, CockroachDB already had full support for marking
arbitrary columns as "hidden" internally: a hidden column can be used
for queries but does not get automatically selected by a `*` in a
SELECT clause, and does not get automatically inserted/upserted into
when INSERT/UPSERT do not list any columns.

However, no control was given to users to determine which columns
would be hidden. The only column that could be hidden in practice was
the implicit `rowid` column created when CREATE does not specify a
primary key.

This patch complements the existing thorough support for hidden column
by properly exposing the "hidden" attribute through SQL:

- any column can now be marked as hidden in CREATE using the column
  attribute `NOT VISIBLE`.
- the attribute can now be changed with `ALTER TABLE .. ALTER COLUMN
  .. SET [NOT] VISIBLE`.

This change is further motivated by the need to annotate non-PK
columns in `pg_catalog` vtables, for compatibility with
PostgreSQL (this will be actually done in a later patch).

The output of `SHOW CREATE` now displays all columns with their
optional `NOT VISIBLE` attribute, instead of skipping over hidden
column as previously.

Note: the choice for the syntax "NOT VISIBLE" as opposed to "HIDDEN"
is mandated by the fact that the first word of an attribute must be a
reserved keyword, and adding reserved keywords is disruptive. This
way, the patch is properly backward-compatible.

Release note (sql change): clients can now set the visibility of a
column using the `NOT VISIBLE` attribute in `CREATE TABLE` or `SET
VISIBLE` / `SET NOT VISIBLE` in `ALTER TABLE .. ALTER COLUMN`.
@knz knz requested a review from BramGruneir June 12, 2018 17:05
@knz knz requested a review from a team as a code owner June 12, 2018 17:05
@knz knz requested review from a team June 12, 2018 17:05
@knz knz requested a review from a team as a code owner June 12, 2018 17:05
@knz knz requested review from a team June 12, 2018 17:05
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz added the docs-todo label Jun 12, 2018
@knz knz force-pushed the 20180612-hidden branch from 49b1a36 to abaf880 Compare June 12, 2018 17:09
@jordanlewis
Copy link
Member

This is cool, but I don't understand why it's necessary to add this feature to support pg_am and so on. Is there really no way to support those tables without introducing a major new schema feature that we'll necessarily have to support forever?


Review status: :shipit: complete! 0 of 0 LGTMs obtained


Comments from Reviewable

@knz
Copy link
Contributor Author

knz commented Jun 12, 2018

This is cool, but I don't understand why it's necessary to add this feature to support pg_am and so on.

  1. pg_am and pg_opclass (and others) are meant to have an oid column that is hidden. It must be hidden because * must not expand it.
  2. our vtables do not have primary keys. Adding them would be major work.
  3. if we announce a non-existing primary key for a vtable in its descriptor but without actually implementing any of its structural constraints, we would need to also pretend it does not exist in all the other virtual tables (pg_index, etc). This check would be ad-hoc and error-prone.
  4. instead, this patch makes it possible to create the vtable descriptor with the proper attribute without ugly hack in the vtable generators.

Is there really no way to support those tables without introducing a [...] new schema feature

There is a way (implement proper PKs in virtual tables) but it's expensive.

[is it not better to avoid introducing] a [...] new schema feature that we'll necessarily have to support forever?

The feature was already there. It was so thoroughly there that we have the proper check if ... col.Hidden ... throughout the code already. This patch is merely revealing this fundamental design aspect of CockroachDB's SQL layer (and postgres') to the users and enables the users to utilize it.

@knz
Copy link
Contributor Author

knz commented Jun 12, 2018 via email

@knz knz added this to the 2.1 milestone Jul 23, 2018
@knz knz closed this May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants