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: do not hide columns in introspection tables #28750

Merged
merged 1 commit into from Aug 19, 2018

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 17, 2018

Fixes #28548.

Requested/suggested by @bdarnell

Clients were confused by the fact some items in introspection referred
to columns that were otherwise not listed because they were marked as
hidden in the table descriptors. For example, an index would
refer to a column (rowid) not listed in the columns for that table.

This has been found (by Ben) to be a problem for SQLAlchemy and (by
me) in dBeaver.

In contrast, PostgreSQL which also has hidden columns always shows all
columns in introspection, including hidden columns, in both
information_schema and pg_catalog.

This patch corrects the issue by doing the same as PostgreSQL. The
output of SHOW COLUMNS is extended to indicate which columns are
hidden.

Release note (bug fix): Hidden columns are now listed in
information_schema and pg_catalog tables, for better compatibility
with PostgreSQL.

Release note (sql change): The output of SHOW COLUMNS is extended to
indicate which columns are hidden.

@knz knz added this to Triage in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Aug 17, 2018
@knz knz requested review from a team August 17, 2018 07:56
@knz knz requested a review from a team as a code owner August 17, 2018 07:56
@knz knz requested review from a team August 17, 2018 07:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz added the docs-todo label Aug 17, 2018
@knz knz force-pushed the 21080817-unhide-info-schema branch from 818a125 to 41b1bf4 Compare August 17, 2018 09:00
@knz knz requested a review from a team as a code owner August 17, 2018 09:00
@knz knz requested a review from a team August 17, 2018 09:00
Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, this looks like it breaks one of the example orms checks...

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

Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM mod comment, thanks!

@@ -254,7 +254,8 @@ CREATE TABLE information_schema.columns (
CHARACTER_SET_CATALOG STRING,
CHARACTER_SET_SCHEMA STRING,
CHARACTER_SET_NAME STRING,
GENERATION_EXPRESSION STRING
GENERATION_EXPRESSION STRING,
IS_HIDDEN STRING NOT NULL -- CockroachDB extension
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a medium-strength objection to this. It's trouble to add things to information_schema unless we absolutely have to. I would prefer to leave information_schema.columns the same, and let the hidden-ness be ambiguous, just like it is in Postgres.

@knz
Copy link
Contributor Author

knz commented Aug 17, 2018

The failing test was because example-orms needed a boost (you approved that PR). The tests should pass now.

I do not understand your concern about information_schema. Everyone seems to place their extensions in here:

What's the trouble exactly?

@jordanlewis
Copy link
Member

My concern is that we're adding pointless additional complexity to the table, which puts additional backward compatibility burden on us. This is pretty low stakes, but I don't want to establish a precedent of adding columns to information_schema unless for some reason we really have to. Postgres doesn't have a hidden column in this, even though they have hidden columns in all their tables (oid, xid, etc) - why should we?

@knz
Copy link
Contributor Author

knz commented Aug 19, 2018

Postgres doesn't have a hidden column in this, even though they have hidden columns in all their tables (oid, xid, etc) - why should we?

Because cockroach dump needs a way to exclude hidden columns when it inspects the schema of a table to dump. If you can find a better way to exclude them I'll take it. If you're really set against it, I'll simply create a new crdb_internal table and call it a day.

@jordanlewis
Copy link
Member

I didn't realize you actually needed the column. In that case, I think I'm fine with it. LGTM!

Requested/suggested by @bdarnell

Clients were confused by the fact some items in introspection referred
to columns that were otherwise not listed because they were marked as
hidden in the table descriptors. For example, an index would
refer to a column (rowid) not listed in the columns for that table.

This has been found (by Ben) to be a problem for SQLAlchemy and (by
me) in dBeaver.

In contrast, PostgreSQL which also has hidden columns always shows all
columns in introspection, including hidden columns, in both
information_schema and pg_catalog.

This patch corrects the issue by doing the same as PostgreSQL.  The
output of SHOW COLUMNS is extended to indicate which columns are
hidden.

Release note (bug fix): Hidden columns are now listed in
`information_schema` and `pg_catalog` tables, for better compatibility
with PostgreSQL.

Release note (sql change): The output of `SHOW COLUMNS` is extended to
indicate which columns are hidden.
@knz knz force-pushed the 21080817-unhide-info-schema branch from 41b1bf4 to 5c7989c Compare August 19, 2018 18:54
@knz knz moved this from Triage to Current milestone in (DEPRECATED) SQL Front-end, Lang & Semantics Aug 19, 2018
@knz
Copy link
Contributor Author

knz commented Aug 19, 2018

thank you! 💖

bors r+

craig bot pushed a commit that referenced this pull request Aug 19, 2018
28750: sql: do not hide columns in introspection tables r=knz a=knz

Fixes #28548.

Requested/suggested by @bdarnell

Clients were confused by the fact some items in introspection referred
to columns that were otherwise not listed because they were marked as
hidden in the table descriptors. For example, an index would
refer to a column (rowid) not listed in the columns for that table.

This has been found (by Ben) to be a problem for SQLAlchemy and (by
me) in dBeaver.

In contrast, PostgreSQL which also has hidden columns always shows all
columns in introspection, including hidden columns, in both
information_schema and pg_catalog.

This patch corrects the issue by doing the same as PostgreSQL.  The
output of SHOW COLUMNS is extended to indicate which columns are
hidden.

Release note (bug fix): Hidden columns are now listed in
`information_schema` and `pg_catalog` tables, for better compatibility
with PostgreSQL.

Release note (sql change): The output of `SHOW COLUMNS` is extended to
indicate which columns are hidden.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Aug 19, 2018

Build succeeded

@craig craig bot merged commit 5c7989c into cockroachdb:master Aug 19, 2018
@knz knz deleted the 21080817-unhide-info-schema branch August 19, 2018 19:23
@knz knz moved this from Current milestone to Finished (milestone 0731) in (DEPRECATED) SQL Front-end, Lang & Semantics Aug 20, 2018
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.

sql: Hide implicit rowid index from introspection interfaces
3 participants