this direction seems ok to go in, thanks

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

pkg/sql/crdb_internal.go line 8580 at r1 (raw file):

CREATE VIEW crdb_internal.kv_system_privileges AS
SELECT username, path,
       crdb_internal.privilege_name(privileges),

nit: use AS to give these well-defined column names

pkg/sql/privilege/kind.go line 74 at r1 (raw file):

// KindKey is the value stored in system tables, etc, that represent
// the privilege internally. It is not visible to end-users.
type KindInternalKey string

why did we get rid of the stringer here? it seems that func (Kind) String() is no longer defined, but it was still used above. also, it seems less error-prone since with this change it's easy for someone to forget to update this switch statement

pkg/sql/privilege/privilege.go line 303 at r1 (raw file):

		case OriginFromUserInput:
			var ok bool
			k, ok = ByDisplayName[KindDisplayName(strings.ToUpper(s))]

don't we need to also keep allowing the user to specify the internal key?

I'm picking up this from Raphael to push it over the finish line.

Reviewed 26 of 26 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @rafiss)

pkg/sql/crdb_internal.go line 8580 at r1 (raw file):

Done.

pkg/sql/privilege/kind.go line 74 at r1 (raw file):
My guess is that the rationale is something like this: the removal of automatically generated String() implementation allows us to provide custom DisplayName() to make user-visible changes as we please. The fact that String() is no longer implemented pushes engineers to choose either InternalKey or DisplayName.

However, it does make it so that our "lintonbuild" config fails to compile because in many (?) places we use privilege.Kind with %s format directive which is no longer valid. @knz can you share your perspective on this before I start updating all those places? (I'm assuming in vast majority we'll be using DisplayName, right?

it seems less error-prone since with this change it's easy for someone to forget to update this switch statement

I don't think it's a concern because in init() below we iterate over all kinds, and for all non-deprecated ones we call InternalKey; thus, if someone forgets to add another case to the switch, the build will be red.

pkg/sql/privilege/kind.go line 84 at r1 (raw file):

Ack, I removed it.

pkg/sql/privilege/privilege.go line 303 at r1 (raw file):

Yes, I believe we need to, done.

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

pkg/sql/privilege/kind.go line 74 at r1 (raw file):

The fact that String() is no longer implemented pushes engineers to choose either InternalKey or DisplayName.

correct.

I'm assuming in vast majority we'll be using DisplayName, right?

correct, but we'd need to be careful for those cases that generate SQL syntax.

pkg/sql/privilege/privilege.go line 303 at r1 (raw file):

Can you double check? I intended the ByDisplayName map to be already initialized (via init function) to contain both names and internal keys.

I think the CI should be green, so this is RFAL, @ecwall @rafiss

Reviewed 2 of 3 files at r3, 1 of 1 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @rafiss)

pkg/sql/privilege/kind.go line 74 at r1 (raw file):

Turns out that there were just a handful of places like this, and in all of them I think DisplayName() is the right choice.

pkg/sql/privilege/privilege.go line 303 at r1 (raw file):

Oh, right, I missed that, reverted my change.

pkg/sql/privilege/privilege.go line 136 at r4 (raw file):

	for i, p := range pl {
		if i > 0 {
			s.SafeString(", ")

Want to bring attention to this change - previously we had privilege.List.String() which joined strings with ", " separator, we removed that implementation, so in some logic tests we started seeing a different error message where multiple privileges were joined just by a comma. Adding a space here should fix those tests, but it also seems like a nice change on its own. Thoughts?

Reviewed 1 of 1 files at r6, 4 of 4 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @rafiss)

pkg/sql/logictest/testdata/logic_test/grant_database line 12 at r7 (raw file):

a              root     ALL             true

statement error user root must have exactly \[ALL\] privileges on database "a"

Here is another minor difference - we now use privilege.List.SortedDisplayNames in a few places which returns []string, and when that gets stringified, we see square brackets around the names. Seems reasonable.

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @knz, and @rafiss)

pkg/sql/crdb_internal.go line 8609 at r8 (raw file):

       crdb_internal.privilege_name(privileges) AS privileges,
       crdb_internal.privilege_name(grant_options) AS grant_options,
       user_id

Hm, there is a difficulty here: user_id column was added to system.privileges during 23.1 cycle, so in mixed 22.2-23.1+ cluster this view might result in an error if it happens to be queried on the older node. I'm not quite sure what to do here since I don't think that we have infrastructure to gate the schema of this virtual view based on the cluster version.

Do we actually need to care about this 22.2-23.1+ cluster state? I don't think we're implementing version skipping from 22.2 to 23.2, so it seems that we actually don't care about this. This would mean that we add a skip for local-mixed-22_2-23_1 logic test config to a couple of files.

The view is currently only used to power SHOW GRANTS which doesn't actually use user_id column, so in theory we could adjust the schema to omit this column. This view was added recently in #104565 and was backported to 22.2 and 23.1.

@ecwall thoughts on this?

Apologies for all the pushes, this is RFAL, @ecwall @rafiss.

Reviewed 2 of 2 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @ericharmeling, @knz, @msbutler, and @rafiss)

pkg/sql/crdb_internal.go line 8609 at r8 (raw file):

I added a skip for local-mixed-22_2-23_1 in two places. This means that SHOW SYSTEM GRANTS might not work in mixed version 22.2-23.2 cluster when issued against the older node (apparently, we'll have experimental support for this). The alternative is to change the schema to remove user_id column, but this would make the virtual table schema differ from the system table, so I chose the skip option.

thank you!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @ericharmeling, @knz, @msbutler, and @yuzefovich)

pkg/sql/crdb_internal.go line 8609 at r8 (raw file):

we no longer need to support mixed 22.2/23.2 clusters, so i guess that's why we didn't end up needing a change here?

pkg/sql/privilege/kind.go line 30 at r10 (raw file):

	CREATE Kind = 2
	DROP   Kind = 3
	// This is a placeholder to make sure that 4 is not reused.

actually, i think it might be safe to reuse 4 at this point. i guess no need to try that unless there's a need.

pkg/sql/privilege/privilege.go line 136 at r4 (raw file):

i agree, thanks

pkg/sql/logictest/testdata/logic_test/grant_database line 12 at r7 (raw file):

+1

TFTR!

bors r+

Reviewed 11 of 11 files at r11, 3 of 3 files at r12, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ecwall, @ericharmeling, @msbutler, and @rafiss)

pkg/sql/crdb_internal.go line 8609 at r8 (raw file):

Right, we no longer need the skip, so I removed a couple from the logic tests.

pkg/sql/privilege/kind.go line 30 at r10 (raw file):

Ack.