sql: support COMMENT ON SCHEMA #68606

ekalinin · 2021-08-09T18:29:10Z

This change adds support for SCHEMA comment.

Release note (sql change): This change adds associating
comment to SQL schema using PostgreSQL's
COMMENT ON SCHEMA syntax.

blathers-crl · 2021-08-09T18:29:13Z

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:

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.

I have added a few people who may be able to assist in reviewing:

@jordanlewis (commented on sql: Support 'COMMENT ON SCHEMA' #67689)
@knz (commented on sql: Support 'COMMENT ON SCHEMA' #67689)

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

cockroach-teamcity · 2021-08-09T18:29:18Z

This change is

blathers-crl · 2021-08-09T18:52:08Z

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.}

blathers-crl · 2021-08-09T19:59:04Z

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.}

blathers-crl · 2021-08-10T07:24:39Z

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.}

blathers-crl · 2021-08-10T19:44:17Z

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.}

blathers-crl · 2021-08-10T20:11:17Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

knz

This PR is missing the necessary changes to sql/pg_catalog.go to generate the values in pg_catalog.pg_description. This will also need a test about how to retrieve the schema comment via pg_catalog.obj_description().

You'll probably need input from someone in the SQL schema team about how to populate pg_description properly.

Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ekalinin)

pkg/sql/drop_schema.go, line 185 at r1 (raw file):

		}

		if err := p.removeSchemaComment(params.ctx, sc.GetID()); err != nil {

This needs to go to dropSchemaImpl otherwise the comment is not dropped upon DROP DATABASE CASCADE

ekalinin · 2021-08-11T19:11:14Z

This PR is missing the necessary changes to sql/pg_catalog.go to generate the values in pg_catalog.pg_description. This will also need a test about how to retrieve the schema comment via pg_catalog.obj_description().

You'll probably need input from someone in the SQL schema team about how to populate pg_description properly.

Thanks. Added.

This needs to go to dropSchemaImpl otherwise the comment is not dropped upon DROP DATABASE CASCADE

Done.

knz

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ekalinin)

pkg/sql/pg_catalog.go, line 1398 at r2 (raw file):

			case keys.SchemaCommentType:
				objID = tree.NewDOid(tree.MustBeDInt(objID))
				classOid = tree.NewDOid(catconstants.PgCatalogSchemaID)

We'll need a review from SQL Schema for this change.
(@ajwerner?)

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5085 at r2 (raw file):

----
objoid  classoid    objsubid  description
94      4294967022  0         testing schema

I think we need to test this via pg_catalog.obj_description() because I don't think we wish to assert particular object IDs inside unit tests.

Someone from SQL experience would know better.
(@rafiss?)

postamar · 2021-08-12T14:37:25Z

pkg/sql/sem/builtins/pg_builtins.go

@@ -1942,6 +1942,8 @@ func getCatalogOidForComments(catalogName string) (id int, ok bool) {
 		return catconstants.PgCatalogClassTableID, true
 	case "pg_database":
 		return catconstants.PgCatalogDatabaseTableID, true
+	case "pg_schema":
+		return catconstants.PgCatalogSchemaID, true


I checked the postgres documentation and I couldn't find any mention of pg_schema in there. Strictly speaking the table for schemas in the pg catalog is pg_namespace. As far as comments are concerned, it seems they might actually belong in pg_shdescription or pg_description. Perhaps @rafiss can enlighten us?

a quick test inside psql tells me that schema comments get stored inside pg_description.

ekalinin · 2021-08-12T18:57:09Z

I think we need to test this via pg_catalog.obj_description() because I don't think we wish to assert particular object IDs inside unit tests.

Fixed.

I checked the postgres documentation and I couldn't find any mention of pg_schema in there. Strictly speaking the table for schemas in the pg catalog is pg_namespace

Fixed.

otan

Reviewed 13 of 14 files at r1, 2 of 6 files at r2, 3 of 4 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ekalinin, @knz, and @rafiss)

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5085 at r2 (raw file):

Previously, knz (kena) wrote…

I think we need to test this via pg_catalog.obj_description() because I don't think we wish to assert particular object IDs inside unit tests.

Someone from SQL experience would know better.
(@rafiss?)

i'd probably omit the oids / subids simply because they are potentially unstable.

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5082 at r3 (raw file):

SELECT obj_description(objoid)
  FROM pg_catalog.pg_description
 WHERE description = 'testing schema';

nit: drop the trailing ; here and above

pkg/sql/logictest/testdata/logic_test/schema, line 881 at r3 (raw file):


statement ok;
USE comment_db;

nit: drop the trailing ; here

pkg/sql/sem/builtins/pg_builtins.go, line 1946 at r2 (raw file):

Previously, knz (kena) wrote…

a quick test inside psql tells me that schema comments get stored inside pg_description.

pg_description is correct

ekalinin

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ekalinin, @knz, and @rafiss)

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5085 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

i'd probably omit the oids / subids simply because they are potentially unstable.

Should I remove test via obj_description?

otan · 2021-08-17T09:36:21Z

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5085 at r2 (raw file):

Previously, ekalinin (Eugene Kalinin) wrote…

Should I remove test via obj_description?

Is there a test for it? We should keep it for sure if there is and add it if it isn't.

ekalinin

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ekalinin, @knz, @otan, and @rafiss)

pkg/sql/drop_schema.go, line 185 at r1 (raw file):

Previously, knz (kena) wrote…

This needs to go to dropSchemaImpl otherwise the comment is not dropped upon DROP DATABASE CASCADE

Fixed.

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5085 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

Is there a test for it? We should keep it for sure if there is and add it if it isn't.

Yes, there's. Please, see the latest version of the PR.

https://github.com/cockroachdb/cockroach/pull/68606/files#diff-44361d449f2d21bb6beb453a14b8475fe98e73a04bf990d251ed99f055001eb8R5080

pkg/sql/logictest/testdata/logic_test/pg_catalog, line 5082 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: drop the trailing ; here and above

Fixed.

pkg/sql/logictest/testdata/logic_test/schema, line 881 at r3 (raw file):

Previously, otan (Oliver Tan) wrote…

nit: drop the trailing ; here

Fixed.

This change adds support for SCHEMA commenting. Release note (sql change): This change adds associating comment to SQL schema using PostgreSQL's COMMENT ON SCHEMA syntax.

ekalinin

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ekalinin, @knz, @otan, and @rafiss)

pkg/sql/sem/builtins/pg_builtins.go, line 1946 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

pg_description is correct

Fixed

otan

looks good to me! thanks

bors r+

craig · 2021-08-18T01:31:14Z

Build succeeded:

GitHub CI (Cockroach)

ekalinin requested a review from a team August 9, 2021 18:29

ekalinin requested a review from a team as a code owner August 9, 2021 18:29

ekalinin requested review from shermanCRL and removed request for a team August 9, 2021 18:29

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Aug 9, 2021

blathers-crl bot requested review from jordanlewis and knz August 9, 2021 18:29

ekalinin force-pushed the comment-on-database branch from f0fe1da to b7e22d1 Compare August 9, 2021 18:52

shermanCRL removed their request for review August 9, 2021 18:53

ekalinin force-pushed the comment-on-database branch from b7e22d1 to bca0d80 Compare August 9, 2021 19:59

ekalinin force-pushed the comment-on-database branch from bca0d80 to dd7d105 Compare August 10, 2021 07:24

ekalinin force-pushed the comment-on-database branch from dd7d105 to f78a4b6 Compare August 10, 2021 19:44

ekalinin force-pushed the comment-on-database branch from f78a4b6 to 9008f08 Compare August 10, 2021 20:11

jordanlewis requested review from a team and removed request for a team, jordanlewis and knz August 10, 2021 20:26

knz reviewed Aug 11, 2021

View reviewed changes

ekalinin force-pushed the comment-on-database branch 2 times, most recently from fa378dd to c02b849 Compare August 11, 2021 19:10

knz reviewed Aug 11, 2021

View reviewed changes

postamar reviewed Aug 12, 2021

View reviewed changes

ekalinin force-pushed the comment-on-database branch from c02b849 to 84b4e6f Compare August 12, 2021 18:52

otan reviewed Aug 17, 2021

View reviewed changes

ekalinin commented Aug 17, 2021

View reviewed changes

ekalinin force-pushed the comment-on-database branch from 84b4e6f to 882e08e Compare August 17, 2021 17:41

ekalinin requested a review from a team as a code owner August 17, 2021 17:41

blathers-crl bot requested a review from otan August 17, 2021 17:41

sql: support COMMENT ON SCHEMA

edc8951

This change adds support for SCHEMA commenting. Release note (sql change): This change adds associating comment to SQL schema using PostgreSQL's COMMENT ON SCHEMA syntax.

ekalinin force-pushed the comment-on-database branch from 882e08e to edc8951 Compare August 17, 2021 17:48

ekalinin commented Aug 17, 2021

View reviewed changes

otan approved these changes Aug 17, 2021

View reviewed changes

craig bot merged commit a60b8bb into cockroachdb:master Aug 18, 2021

jseldess mentioned this pull request Sep 8, 2021

sql: support COMMENT ON SCHEMA cockroachdb/docs#11428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: support COMMENT ON SCHEMA #68606

sql: support COMMENT ON SCHEMA #68606

ekalinin commented Aug 9, 2021

blathers-crl bot commented Aug 9, 2021

cockroach-teamcity commented Aug 9, 2021

blathers-crl bot commented Aug 9, 2021

blathers-crl bot commented Aug 9, 2021

blathers-crl bot commented Aug 10, 2021

blathers-crl bot commented Aug 10, 2021

blathers-crl bot commented Aug 10, 2021

knz left a comment

ekalinin commented Aug 11, 2021

knz left a comment

postamar Aug 12, 2021

knz Aug 12, 2021

ekalinin commented Aug 12, 2021

otan left a comment

ekalinin left a comment

otan commented Aug 17, 2021

ekalinin left a comment

ekalinin left a comment

otan left a comment

craig bot commented Aug 18, 2021

sql: support COMMENT ON SCHEMA #68606

sql: support COMMENT ON SCHEMA #68606

Conversation

ekalinin commented Aug 9, 2021

blathers-crl bot commented Aug 9, 2021

cockroach-teamcity commented Aug 9, 2021

blathers-crl bot commented Aug 9, 2021

blathers-crl bot commented Aug 9, 2021

blathers-crl bot commented Aug 10, 2021

blathers-crl bot commented Aug 10, 2021

blathers-crl bot commented Aug 10, 2021

knz left a comment

Choose a reason for hiding this comment

ekalinin commented Aug 11, 2021

knz left a comment

Choose a reason for hiding this comment

postamar Aug 12, 2021

Choose a reason for hiding this comment

knz Aug 12, 2021

Choose a reason for hiding this comment

ekalinin commented Aug 12, 2021

otan left a comment

Choose a reason for hiding this comment

ekalinin left a comment

Choose a reason for hiding this comment

otan commented Aug 17, 2021

ekalinin left a comment

Choose a reason for hiding this comment

ekalinin left a comment

Choose a reason for hiding this comment

otan left a comment

Choose a reason for hiding this comment

craig bot commented Aug 18, 2021