sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns #58923

Heoric · 2021-01-13T09:16:53Z

Release note (sql change): It is now possible to use the NOT VISIBLE
qualifier for new column definitions in CREATE TABLE. This causes the
column to become 'hidden'. Hiddens columns are not considered when
using * in SELECT clauses.

(Note that CockroachDB already supported hidden columns previously,
such as rowid which is added automatically when a table definition
has no PRIMARY KEY constraint. This change merely adds the opportunity
for end-users to define their own hidden columns.)

This feature is intended for use in combination with other features
related to geo-partitioning introduced in v21.1, to offer more control
about how geo-partitioning keys get exposed to client ORMs and other
automated tools that inspect the SQL schema.

cockroach-teamcity · 2021-01-13T09:16:56Z

All committers have signed the CLA.

blathers-crl · 2021-01-13T09:16:57Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

A person you worked with closely on this PR.
The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
Join our community slack channel and ask on #contributors.
Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2021-01-13T09:17:00Z

This change is

ajwerner · 2021-01-14T05:34:59Z

This happens to be a concept we're explicitly exploring right now in this RFC: #58440

knz

Thank you, this actually looks pretty good.

You will need a proper commit message however, that explains the work (motivation and approach).
Also the PR description needs to indicate the link to the motivating issue, e.g. "fixes #53428"
As well as a release note that outlines the user-facing changes.
For more information about how to write a proper commit message, head here: https://wiki.crdb.io/wiki/spaces/CRDB/pages/73072807/Git+Commit+Messages

Also this PR is missing unit tests. There needs to be some tests in sql/parser/parse_test.go, and also some tests in pkg/sql/logictests

I invite you to take inspiration from my previous PR #26644 which also contains unit tests (you can take most of them over). I'd, of course, also appreciate a reference to that previous work in your commit message.

Reviewed 13 of 13 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

pkg/sql/show_create.go, line 87 at r1 (raw file):

	f.WriteString(" (")
	primaryKeyIsOnVisibleColumn := false
	// visibleCols := desc.VisibleColumns()

I don't understand this - why did you need to change it?

pkg/sql/parser/sql.y, line 1365 at r1 (raw file):

//
// Column qualifiers:
//   [CONSTRAINT <constraintname>] {NULL | NOT NULL | VISIBLE | UNIQUE [WITHOUT INDEX] | PRIMARY KEY | CHECK (<expr>) | DEFAULT <expr>}

probably | [NOT] VISIBLE ... here

pkg/sql/parser/sql.y, line 1901 at r1 (raw file):

  }
  // ALTER TABLE <name> ALTER [COLUMN] <colname> SET NOT VISIBLE
| ALTER opt_column column_name SET NOT VISIBLE

please add the corresponding tests in parse_test.go thanks

pkg/sql/parser/sql.y, line 5655 at r1 (raw file):

//
// Column qualifiers:
//   [CONSTRAINT <constraintname>] {NULL | NOT NULL | VISIBLE | NOT VISIBLE |UNIQUE [WITHOUT INDEX] | PRIMARY KEY | CHECK (<expr>) | DEFAULT <expr>}

make this | [NOT] VISIBLE | UNIQUE ...

pkg/sql/sem/tree/create.go, line 600 at r1 (raw file):

		ctx.FormatNode(&node.Nullable.ConstraintName)
	}
	if node.Hidden {

Can you also update (node *ColumnTableDef) docRow() accordingly in pretty.go, thanks.

pkg/sql/vtable/pg_catalog.go, line 21 at r1 (raw file):

const PGCatalogAm = `
CREATE TABLE pg_catalog.pg_am (
	oid OID NOT VISIBLE,

This change, while welcome, belongs to a different commit/PR. Please split it away from the first commit, thanks.

knz · 2021-01-14T10:00:22Z

@otan @rafiss I think this should be on your radar.

thoszhang · 2021-01-14T16:25:17Z

I'd like to request that you split the ALTER TABLE ... ALTER COLUMN change into a separate PR to be reviewed after this one, and limit the scope of this PR to specifying hidden columns in CREATE TABLE and ALTER TABLE ... ADD COLUMN (and writing tests for those statements). Changing the visibility of columns is a more peripheral use case, and schema changes can require some care.

blathers-crl · 2021-01-15T07:42:09Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

blathers-crl · 2021-01-15T07:44:59Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

blathers-crl · 2021-01-15T09:26:51Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

knz

Thanks for updating the PR.

@lucy-zhang what should we do regarding ALTER? Do we ask to remove the SQL parser support entirely, or should we keep the syntax but then report an unimplemented error during planning?

Reviewed 11 of 11 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

pkg/sql/parser/parse_test.go, line 1452 at r2 (raw file):

		{`ALTER TABLE a ALTER COLUMN b DROP NOT NULL`},
		{`ALTER TABLE a ALTER COLUMN b DROP STORED`},
		{`ALTER TABLE a ALTER COLUMN b SET VISIBLE`},

I think you can remove the ALTER tests here now.

pkg/sql/parser/sql.y, line 1346 at r2 (raw file):

//   ALTER TABLE ... ALTER [COLUMN] <colname> DROP NOT NULL
//   ALTER TABLE ... ALTER [COLUMN] <colname> DROP STORED
//   ALTER TABLE ... ALTER [COLUMN] <colname> SET [NOT] VISIBLE

Since you are extracting the ALTER functionality into a different PR, you can remove this from this PR too.

pkg/sql/parser/sql.y, line 1901 at r2 (raw file):

  }
  // ALTER TABLE <name> ALTER [COLUMN] <colname> SET NOT VISIBLE
| ALTER opt_column column_name SET NOT VISIBLE

same here.

pkg/sql/sem/tree/alter_table.go, line 76 at r2 (raw file):

func (*AlterTableSetAudit) alterTableCmd()           {}
func (*AlterTableSetDefault) alterTableCmd()         {}
func (*AlterTableSetHidden) alterTableCmd()          {}

same here

Heoric · 2021-01-19T01:32:38Z

Should I just commit the create table... and split the ALTER TABLE ... ALTER COLUMN syntax and implementation to another PR? @lucy-zhang @knz

blathers-crl · 2021-01-21T08:51:57Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

blathers-crl · 2021-01-21T09:42:50Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

knz

I think this looks good now.
Lucy do you want to take a look and merge this if you're satisfied?

Reviewed 3 of 5 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

knz · 2021-01-21T13:31:29Z

@Heoric sorry I forgot - you need to populate a commit message that explains the change, according to the instructions in the wiki (see my link above). Also you can squash the 3 commits together.

Heoric · 2021-01-22T05:44:29Z

Thank you for your advice and look forward to your review. @lucy-zhang

Heoric · 2021-01-22T07:25:10Z

Thank you very much for the review!

…

------------------ 原始邮件 ------------------ 发件人: "kena"<[email protected]>; 发送时间: 2021年1月21日(星期四) 晚上9:30 收件人: "cockroachdb/cockroach"<[email protected]>; 抄送: "奔跑"<[email protected]>; "Mention"<[email protected]>; 主题: Re: [cockroachdb/cockroach] [WIP] user-defined hidden columns (#58923) @knz approved this pull request. I think this looks good now. Lucy do you want to take a look and merge this if you're satisfied? Reviewed 3 of 5 files at r3, 2 of 2 files at r4. Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

thoszhang

Reviewed 4 of 13 files at r1, 1 of 11 files at r2, 2 of 5 files at r3, 2 of 2 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

pkg/sql/logictest/testdata/logic_test/hidden_columns, line 1 at r4 (raw file):

statement ok

Can you add logic tests for these interactions with DDL statements:

dropping a hidden column
renaming a hidden column
adding an index on a hidden column
adding a foreign key constraint on a hidden column
adding a foreign key constraint that references a hidden column
ALTER PRIMARY KEY on a primary key with hidden columns

I expect these to all work, but it'll be good to make sure.

blathers-crl · 2021-01-27T02:11:16Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

thoszhang

Reviewed 1 of 5 files at r3, 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

pkg/sql/logictest/testdata/logic_test/hidden_columns, line 70 at r5 (raw file):


statement ok
ALTER TABLE kv Drop COLUMN x;

nit: capitalize DROP

pkg/sql/logictest/testdata/logic_test/hidden_columns, line 75 at r5 (raw file):

SELECT x FROM kv;

nit: Only 1 newline between these cases

pkg/sql/logictest/testdata/logic_test/hidden_columns, line 82 at r5 (raw file):


statement ok
CREATE TABLE t2(b INT NOT VISIBLE, c INT, d INT, PRIMARY KEY (d), FOREIGN KEY(b) REFERENCES t1(b));

Can you add some queries after all these schema changes to help make sure that the new descriptor states are usable?

pkg/sql/logictest/testdata/logic_test/hidden_columns, line 103 at r5 (raw file):

nit: Extra newlines at the end (the linter doesn't pick this up)

Heoric · 2021-01-29T01:35:00Z

This has a error when I add a foreign key constraint that references a hidden column(including rowid)。
Is this reasonable?
Should I change it or keep the original error report？ @lucy-zhang @knz

ERROR: validate fk constraint: column "t.rowid" does not exist

CREATE TABLE t3(a INT PRIMARY KEY);
CREATE TABLE t4(c INT);
CREATE TABLE t5(a INT);
ALTER TABLE t4 ADD FOREIGN KEY (rowid) REFERENCES t3 (a);  -- success
ALTER TABLE t4 ADD FOREIGN KEY (c) REFERENCES t5 (rowid);  -- error
CREATE TABLE t6(c INT, FOREIGN KEY(c) REFERENCES t5(rowid)); -- success

knz · 2021-01-29T13:05:13Z

@lucy-zhang this error surprises me too. What do you think?

thoszhang · 2021-01-29T16:11:49Z

That error is coming from executing the internal query generated here:

cockroach/pkg/sql/check.go

Line 152 in 3e2a4e7

func nonMatchingRowQuery(

I reproduced this on master and got the query from the logs:

root@:26257/defaultdb> CREATE TABLE t4(c INT);
CREATE TABLE
root@:26257/defaultdb> CREATE TABLE t5(a INT);
CREATE TABLE
root@:26257/defaultdb> ALTER TABLE t4 ADD FOREIGN KEY (c) REFERENCES t5 (rowid);
ERROR: validate fk constraint: column "t.rowid" does not exist
SQLSTATE: 42703

I210129 15:59:27.481167 3052 sql/check.go:290 ⋮ [n1,job=628705089054212097,scExec,id=52,mutation=1] 118  validating FK ‹"fk_c_ref_t5"› (‹"t4"› [‹[c rowid]›] -> ‹"t5"› [‹[rowid]›]) with query ‹"SELECT s.c, s.rowid FROM \n\t\t  (SELECT c, rowid FROM [52 AS src]@{IGNORE_FOREIGN_KEYS} WHERE c IS NOT NULL) AS s\n\t\t\tLEFT OUTER JOIN\n\t\t\t(SELECT * FROM [53 AS target]) AS t\n\t\t\tON s.c = t.rowid\n\t\t WHERE t.rowid IS NULL  LIMIT 1"›

Here it is cleaned up a bit:

SELECT s.c, s.rowid
  FROM (SELECT c, rowid FROM [52 AS src]@{IGNORE_FOREIGN_KEYS} WHERE c IS NOT NULL) AS s
       LEFT JOIN (SELECT * FROM [53 AS target]) AS t ON s.c = t.rowid
 WHERE t.rowid IS NULL
 LIMIT 1;

This query produces the same error when issued from the sql console, so it's not a problem with the internal executor or anything. I'll ask around and probably file a separate issue, but I don't think that should block this PR.

blathers-crl · 2021-02-01T02:03:14Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

thoszhang

Thanks, changes LGTM. Could you please squash your commits?

Reviewed 1 of 1 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

blathers-crl · 2021-02-02T02:09:58Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

Please ensure your git commit message contains a release note.
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

knz

Ok this is looking good now, but the commit message is imperfect.
@Heoric I will take care of editing the commit message for you. Thank you for your change!

Reviewed 1 of 1 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Heoric)

Release note (sql change): It is now possible to use the `NOT VISIBLE` qualifier for new column definitions in CREATE TABLE. This causes the column to become 'hidden'. Hiddens columns are not considered when using `*` in SELECT clauses. (Note that CockroachDB already supported hidden columns previously, such as `rowid` which is added automatically when a table definition has no PRIMARY KEY constraint. This change merely adds the opportunity for end-users to define their own hidden columns.) This feature is intended for use in combination with other features related to geo-partitioning introduced in v21.1, to offer more control about how geo-partitioning keys get exposed to client ORMs and other automated tools that inspect the SQL schema.

knz · 2021-02-03T12:10:20Z

bors r=lucy-zhang,knz

craig · 2021-02-03T12:45:30Z

Build succeeded:

GitHub CI (Cockroach)

kurokochin · 2021-04-10T14:18:42Z

@otan thanks for covering me! sorry, this week is quite a hectic week for me as I become the on-call for my team, hopefully, in my next contribution I'll work from the start till the end.

blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels Jan 13, 2021

Heoric changed the title ~~user-defined hidden columns~~ [WIP] user-defined hidden columns Jan 14, 2021

knz reviewed Jan 14, 2021

View reviewed changes

knz reviewed Jan 15, 2021

View reviewed changes

Heoric marked this pull request as ready for review January 21, 2021 08:53

knz approved these changes Jan 21, 2021

View reviewed changes

knz changed the title ~~[WIP] user-defined hidden columns~~ sql: enable CREATE to define columns as hidden Jan 21, 2021

thoszhang reviewed Jan 26, 2021

View reviewed changes

thoszhang reviewed Jan 28, 2021

View reviewed changes

thoszhang mentioned this pull request Jan 29, 2021

sql: error on internal validation query for foreign keys referencing hidden columns #59582

Closed

thoszhang approved these changes Feb 1, 2021

View reviewed changes

knz reviewed Feb 3, 2021

View reviewed changes

knz changed the title ~~sql: enable CREATE to define columns as hidden~~ sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns Feb 3, 2021

knz mentioned this pull request Feb 3, 2021

sql: permit user-defined hidden columns #53428

Closed

6 tasks

craig bot merged commit 78bda6c into cockroachdb:master Feb 3, 2021

jseldess mentioned this pull request Mar 23, 2021

sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns cockroachdb/docs#10057

Closed

otan mentioned this pull request Apr 1, 2021

sql: implement ALTER TABLE ... ALTER COLUMN SET [VISIBLE|NOT VISIBLE] #62892

Closed

Heoric deleted the issue-#53428 branch September 23, 2021 09:31

wenyihu6 mentioned this pull request Jun 29, 2022

rfc: add rfc for invisible index feature #83531

Merged

rafiss mentioned this pull request Sep 2, 2023

roachtest: acceptance/version-upgrade failed #109562

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns #58923

sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns #58923

Heoric commented Jan 13, 2021 •

edited by knz

Loading

cockroach-teamcity commented Jan 13, 2021 •

edited

Loading

blathers-crl bot commented Jan 13, 2021

cockroach-teamcity commented Jan 13, 2021

ajwerner commented Jan 14, 2021

knz left a comment

knz commented Jan 14, 2021

thoszhang commented Jan 14, 2021

blathers-crl bot commented Jan 15, 2021

blathers-crl bot commented Jan 15, 2021

blathers-crl bot commented Jan 15, 2021

knz left a comment

Heoric commented Jan 19, 2021

blathers-crl bot commented Jan 21, 2021

blathers-crl bot commented Jan 21, 2021

knz left a comment

knz commented Jan 21, 2021

Heoric commented Jan 22, 2021

Heoric commented Jan 22, 2021 via email

thoszhang left a comment

blathers-crl bot commented Jan 27, 2021

thoszhang left a comment

Heoric commented Jan 29, 2021 •

edited

Loading

knz commented Jan 29, 2021

thoszhang commented Jan 29, 2021

blathers-crl bot commented Feb 1, 2021

thoszhang left a comment

blathers-crl bot commented Feb 2, 2021

knz left a comment

knz commented Feb 3, 2021

craig bot commented Feb 3, 2021

kurokochin commented Apr 10, 2021

sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns #58923

sql: support NOT VISIBLE in CREATE TABLE for user-defined hidden columns #58923

Conversation

Heoric commented Jan 13, 2021 • edited by knz Loading

cockroach-teamcity commented Jan 13, 2021 • edited Loading

blathers-crl bot commented Jan 13, 2021

cockroach-teamcity commented Jan 13, 2021

ajwerner commented Jan 14, 2021

knz left a comment

Choose a reason for hiding this comment

knz commented Jan 14, 2021

thoszhang commented Jan 14, 2021

blathers-crl bot commented Jan 15, 2021

blathers-crl bot commented Jan 15, 2021

blathers-crl bot commented Jan 15, 2021

knz left a comment

Choose a reason for hiding this comment

Heoric commented Jan 19, 2021

blathers-crl bot commented Jan 21, 2021

blathers-crl bot commented Jan 21, 2021

knz left a comment

Choose a reason for hiding this comment

knz commented Jan 21, 2021

Heoric commented Jan 22, 2021

Heoric commented Jan 22, 2021 via email

thoszhang left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Jan 27, 2021

thoszhang left a comment

Choose a reason for hiding this comment

Heoric commented Jan 29, 2021 • edited Loading

knz commented Jan 29, 2021

thoszhang commented Jan 29, 2021

blathers-crl bot commented Feb 1, 2021

thoszhang left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Feb 2, 2021

knz left a comment

Choose a reason for hiding this comment

knz commented Feb 3, 2021

craig bot commented Feb 3, 2021

kurokochin commented Apr 10, 2021

Heoric commented Jan 13, 2021 •

edited by knz

Loading

cockroach-teamcity commented Jan 13, 2021 •

edited

Loading

Heoric commented Jan 29, 2021 •

edited

Loading