sql/delegate: added object_type and object_name to show grants

[Dedej-Bergin]

Previously, we used the relation_name column to show all objects however we now have objects such as function and types that are not relations so the name of this column was changed to object_name in these code changes. Additionally with the larger variety in types a new column object_type is added that identifies the type of an object.

Fixes: #119775
Release note (sql change): object_type can now be seen with the show grants command and relation_name column is now shown as object_name.

[cockroach-teamcity]

This change is 

[rafiss]

nice work! it looks like you need to update a few other logic tests. the test results in the CI can tell you which files to adjust

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Dedej-Bergin)

---

pkg/sql/delegate/show_grants.go line 66 at r1 (raw file):

  FROM "".information_schema.table_privileges tp
  LEFT JOIN
       "".information_schema.sequences s ON tp.table_name = s.sequence_name`

unfortunately we can't just join on name here. it is possible for the table name and sequence name to be the same if they are in different schemas.

for example:

CREATE SCHEMA s1;
CREATE SCHEMA s2;
CREATE TABLE s1.abc (a INT);
CREATE SEQUENCE s2.abc;

so we would want

LEFT JOIN "".information_schema.sequences s ON (
  tp.table_catalog = s.sequence_catalog AND
  tp.table_schema = s.sequence_schema AND
  tp.table_name = s.sequence_name
)

---

pkg/sql/delegate/show_grants.go line 93 at r1 (raw file):

SELECT *
  FROM (
        SELECT name AS connection_name,

nit: i think we should also add the object_type column here.

---

pkg/sql/delegate/show_grants.go line 339 at r1 (raw file):

		}
	} else {
		nameCols = "database_name, schema_name, object_type, object_name,"

nit: let's put object_type after object_name. it's more natural for all the names to be next to each other in the output

---

pkg/sql/delegate/show_grants.go line 361 at r1 (raw file):

			`SELECT database_name, schema_name, object_type, routine_signature AS object_name, grantee, privilege_type, is_grantable FROM (`)
		source.WriteString(routineQuery)
		source.WriteByte(')')

this is not related to the original issue, but this part of the code should also add a UNION ALL with the externalConnectionsQuery. i think we should just go ahead and fix it now, but let's do it in a different PR

---

pkg/sql/logictest/testdata/logic_test/role line 1892 at r1 (raw file):

grant usage on sequence test_sequence to roach;

query TTTTTTB colnames,rowsort

nit: add a comment to say that the purpose of this test is to verify the object_type column

---

pkg/sql/logictest/testdata/logic_test/role line 1893 at r1 (raw file):

query TTTTTTB colnames,rowsort
select * from [show grants for roach]

nit: there's no advantage in doing select * from [show grants for roach] versus just show grants from roach

the main reason to use the [] syntax here is if we only care about certain column names, like: select object_type, object_name, grantee from [show grants for roach]. this is useful so that the test output doesn't get too big to read easily, so we might want to make that change to only look at relevant columns here.

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

[Dedej-Bergin]

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

---

pkg/sql/delegate/show_grants.go line 66 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

unfortunately we can't just join on name here. it is possible for the table name and sequence name to be the same if they are in different schemas.

for example:

CREATE SCHEMA s1;
CREATE SCHEMA s2;
CREATE TABLE s1.abc (a INT);
CREATE SEQUENCE s2.abc;

so we would want

LEFT JOIN "".information_schema.sequences s ON (
  tp.table_catalog = s.sequence_catalog AND
  tp.table_schema = s.sequence_schema AND
  tp.table_name = s.sequence_name
)

Thanks Rafi, I went ahead and made this change!

---

pkg/sql/delegate/show_grants.go line 93 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: i think we should also add the object_type column here.

Thanks Rafi, I was thinking to add object_type for external connections on the next pull request to fix this PR: #122199 ... if that's okay? Otherwise I can add it here.

---

pkg/sql/delegate/show_grants.go line 339 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: let's put object_type after object_name. it's more natural for all the names to be next to each other in the output

Sounds good, definitely makes more sense to have object_type after object_name. I went ahead and made this change, thanks!

---

pkg/sql/delegate/show_grants.go line 361 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this is not related to the original issue, but this part of the code should also add a UNION ALL with the externalConnectionsQuery. i think we should just go ahead and fix it now, but let's do it in a different PR

Thanks Rafi, I'll do it in the next PR: #122199 since it's already opened, but next time for something similar I'll make sure not to open another PR for it.

---

pkg/sql/logictest/testdata/logic_test/role line 1892 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: add a comment to say that the purpose of this test is to verify the object_type column

I added the comment.

---

pkg/sql/logictest/testdata/logic_test/role line 1893 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: there's no advantage in doing select * from [show grants for roach] versus just show grants from roach

the main reason to use the [] syntax here is if we only care about certain column names, like: select object_type, object_name, grantee from [show grants for roach]. this is useful so that the test output doesn't get too big to read easily, so we might want to make that change to only look at relevant columns here.

It's definitely better to not have the select * part, I went ahead and made the change, thanks Rafi!

[rafiss]

lgtm! nice work

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball and @dt)

[Dedej-Bergin]

bors r+

[craig]

Build failed (retrying...):

• check_generated_code

[craig]

Build succeeded:

• linux_arm64_build
• linux_amd64_build
• local_roachtest
• lint
• acceptance
• macos_amd64_build
• linux_amd64_fips_build
• docker_image_amd64
• windows_build
• examples_orms
• check_generated_code
• local_roachtest_fips
• macos_arm64_build
• Bazel Essential CI (Cockroach)

[Dedej-Bergin]

blathers backport 23.1

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from a9fde23 to blathers/backport-release-23.1-122197: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.

---

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

[Dedej-Bergin]

blathers backport 23.2

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from a9fde23 to blathers/backport-release-23.2-122197: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2 failed. See errors above.

---

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