Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

pkg/sql/exec_util.go, line 2422 at r3 (raw file):

}

func (m *sessionDataMutator) SetRole(ctx context.Context, pc permissionsChecker, s string) error {

can s be a security.SQLUsername so that it's normalzed earlier?

specifically, what happens if the value of s is NoNe? the if s == security.NoneRole check will fail it seems

pkg/sql/exec_util.go, line 2454 at r3 (raw file):

		if !exists {
			return pgerror.Newf(
				pgcode.UndefinedObject,

postgres returns InvalidParameterValue for this

pkg/sql/vars.go, line 1000 at r3 (raw file):

	},

	// See https://www.postgresql.org/docs/current/sql-set-role.html.

do you think we should modify the SetVar formatting logic so that it capitalizes ROLE? if so, add a ParseDataDriven test

also, just curious about the decision to implement this as a session variable rather than as a separate command (it seems Postgres has a separate SET ROLE command?)

pkg/sql/vars.go, line 1001 at r3 (raw file):

	// See https://www.postgresql.org/docs/current/sql-set-role.html.
	`role`: {

could you add role as one of the variables disallowed to be configured using ALTER ROLE ... SET

here:

pkg/sql/logictest/testdata/logic_test/set_role, line 200 at r3 (raw file):

# Revoke admin but give testuser privileges for testuser2.
# Make a testrole role.
# Check testuser2 can become testuser and testrole as they are still

maybe too edge casey to worry about ... but what happens if user_a is an admin, then becomes user_b, and then admin is revoked from user_a. i'm guessing PG doesn't auto-undo the SET ROLE? but then at this point can you still do SET ROLE user_a` successfully?

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

pkg/sql/exec_util.go, line 2422 at r3 (raw file):

Done.

pkg/sql/exec_util.go, line 2454 at r3 (raw file):

Done.

pkg/sql/vars.go, line 1000 at r3 (raw file):

i'm not sure it does.
i don't think postgres has a separate SET ROLE command, it treats it like a var.

pkg/sql/vars.go, line 1001 at r3 (raw file):

Done.

pkg/sql/logictest/testdata/logic_test/set_role, line 200 at r3 (raw file):

just tested, pg doesn't reset the role, and you can no longer do set role user_a. added a test.

Nothing major from me.

Reviewed 2 of 5 files at r2, 1 of 16 files at r3, 1 of 5 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan and @rafiss)

pkg/sql/exec_util.go, line 2422 at r4 (raw file):

}

func (m *sessionDataMutator) SetRole(

Could I request some commentary here that talks about the SessionUserProto and its relationship to UserProto?

pkg/sql/exec_util.go, line 2502 at r4 (raw file):

	if sessionUser.IsRootUser() {

does this let you become node? That thing is more powerful than root. I'm not sure how I feel about making it easy for root to become node.

pkg/sql/vars.go, line 59 at r4 (raw file):

// permissionsCheck contains permissions checks which are needed for SET ROLE.
type permissionsChecker interface {
	MemberOfWithAdminOption(

do y'all ever dream of package boundaries around these sorts of things?

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

pkg/sql/exec_util.go, line 2422 at r4 (raw file):

added a comment here, the protobuf variable has more commentary.

pkg/sql/exec_util.go, line 2502 at r4 (raw file):

does node exist? i don't see it in system.roles

root@127.0.0.1:26257/movr> set role node;
ERROR: role node does not exist
SQLSTATE: 22023

pkg/sql/vars.go, line 59 at r4 (raw file):

🙃

just a small comment but this looks good!

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

pkg/sql/exec_util.go, line 2502 at r4 (raw file):

node is a magic pseudo-role, and so is public.

as long as the set role implementation works by consulting the system table, it wouldn't allow you to become those roles, but maybe worth adding a test or special-casing them.

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

pkg/sql/vars.go, line 1000 at r3 (raw file):

i see. i just saw that it got its own page: https://www.postgresql.org/docs/current/sql-set-role.html

so from the parser perspective, i think it still would be useful to add a ParseDataDriven test for:

• the ROLE token is capitalized
• don't need a = or TO token in the SET ROLE username command

Commentary nits from me. Otherwise

Reviewed 3 of 27 files at r6, 2 of 14 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @otan, and @rafiss)

pkg/sql/user.go, line 461 at r8 (raw file):

	// Buffer the ParamStatusUpdate.
	updateStr := "off"
	willBecomeAdmin, err := p.UserHasAdminRole(ctx, becomeUser)

If we're already an admin, should we be sending this?

pkg/sql/user.go, line 470 at r8 (raw file):

	m.paramStatusUpdater.BufferParamStatusUpdate("is_superuser", updateStr)

	// The "none" user does a reset if we are in a SET ROLE.

Is this reset terminology defined anywhere? Could we make it a method on the sessionDataMutator and put some nice commentary on that method?

pkg/sql/user.go, line 478 at r8 (raw file):

		return nil
	}

Should we short-circuit in the case where we're becoming ourself?

pkg/sql/sessiondata/session_data.go, line 129 at r8 (raw file):

}

// SessionUser retrieves the session_user.

This feels like a nice place to reiterate the distinction between SessionUser and User.

pkg/sql/sessiondatapb/session_data.go, line 111 at r8 (raw file):

}

// User retrieves the current user.

Given this change, it's probably worth a sentence pointing out that SessionUser exists and is distinct from this user. I have a feeling that in general you want to use this and not that. Commenting both places with hints feels valuable.