sql: user/role refactor to match postgres

[RichardJCai]

Refactoring Users and Roles to be the same. (Same as PG)

Release note (sql-feature): CREATE ROLE / CREATE USER are now the same except:
CREATE ROLE would have login disabled by default and CREATE USER has login enabled by default.
(login privilege is not added in this PR). Similarly ALTER/DROP ROLE and USER are the same.

CREATE/ALTER/DROP ROLE syntax are still disabled for users despite CREATE USER being an alias for them.
Similarly DROP / ALTER are the same for roles and users.
Done to match PG behaviour.

---

Discussion:

Questions for product:
Need to determine how to handle enterprise / non enterprise features.
Would it still make sense to disable roles for non enterprise?
One option is to disable user level privileges (CREATEROLE/LOGIN) and GRANTING users/roles to users/roles (This is still currently disabled).

Is it necessary to disable creation of "roles" (the syntax CREATE ROLE) if GRANTING users/roles is disabled?

CREATE USER / ROLE is effectively the same thing.
(In postgres - CREATE USER is an alias for CREATE ROLE)

What should SHOW USERS / SHOW ROLES do? (Equivalent?)

Edit:
After some discussion, we decided CREATE/ALTER/DROP ROLE will still be disabled for non-enterprise to avoid confusion (still cannot GRANT) "roles" don't really exist.
SHOW USERS will show roles/users that can login.
SHOW ROLES will show all roles and users.

• Will handle the SHOW logic in the LOGIN option PR since it depends on the LOGIN flag.

[cockroach-teamcity]

This change is 

[solongordon]

I took a first pass. Looks like this is headed in a good direction. Main points I want to look at:

• Should we keep CREATE USER and CREATE ROLE separate at the syntax level?
• Is it possible to reuse KVOption instead of adding OptionWithValue?

Reviewed 33 of 33 files at r1, 34 of 34 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai)

---

docs/generated/settings/settings.html, line 0 at r2 (raw file):
Looks like this file went away for some reason?

---

pkg/ccl/logictestccl/testdata/logic_test/role, line 53 at r1 (raw file):

statement error pq: user myrole does not exist
DROP USER myrole

Maybe we should still have a test for this if we intend for roles to be droppable with DROP USER?

---

pkg/ccl/roleccl/role.go, line 116 at r2 (raw file):

		// TODO(mberhault): just like GRANT/REVOKE privileges, we fetch the list of all users.
		// This is wasteful when we have a LOT of users compared to the number of users being operated on.
		users, err := p.GetAllUsersAndRoles(ctx)

Should we just change the function name to GetAllRoles? And does it still need to return whether each is a user or a role?

---

pkg/ccl/roleccl/role.go, line 125 at r2 (raw file):

		// Check roles: these have to be roles.
		// Updated to match pg, can be roles or users.

You can just update the comment.

---

pkg/sql/backfill.go, line 834 at r2 (raw file):

		// since they just add their keys blindly. Running a Scan of the target
		// spans at the time the SSTs' keys will be written will calcify history up
		// to then since the scan will resolveUsername intents and populate tscache to keep

Bad rename? Many more examples of this in other files too. I won't comment on them all.

---

pkg/sql/database.go, line 237 at r2 (raw file):

// getCachedDatabaseID returns the ID of a database given its name
// from the cache. This method never goes to the store to resolveUsername

Bad rename

---

pkg/sql/parser/help_test.go, line 69 at r2 (raw file):

		{`ALTER USER foo WITH PASSWORD ??`, `ALTER USER`},

		{`ALTER ROLE bleh ?? WITH NOCREATEROLE`, `ALTER USER`},

This looks weird. If anything I would expect to show the ALTER ROLE help for both ALTER ROLE and ALTER USER. Though it might be nicer to keep them separate.

---

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

			`CREATE USER IF NOT EXISTS 'foo'`},
		{`CREATE USER foo PASSWORD bar`,
			`CREATE USER 'foo' PASSWORD 'bar'`},

I still think it's fine if we just insert the WITH when it's not specified. shrug

---

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

    return u.val.(tree.OptionsWithValues)
}
func (u *sqlSymUnion) isRole() bool {

I don't think you need to add this. You can just use the existing bool() method.

---

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

// %Text: CREATE USER [IF NOT EXISTS] <name> [WITH] <OPTIONS...>
// %SeeAlso: ALTER ROLE, DROP ROLE, SHOW ROLES, ALTER USER, DROP USER, SHOW USER
create_user_or_role_stmt:

It's unclear to me whether we should be merging these statements at the syntax level. CREATE USER is still slightly different from CREATE ROLE and we probably want to show different help text for the two. Do you think perhaps we should keep them separate here even though the underlying implementation is going to be the same?

---

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

    $$.val = &tree.CreateUserOrRole{Name: $6.expr(), IsRole: $2.isRole(), IfNotExists: true}
  }
| CREATE role_or_group_or_user string_or_placeholder role_options

I think you should be able to avoid having separate cases for no options, role_options, and WITH role_options. Check out how opt_with_options is defined.

---

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

USER { $$.val = false }
| ROLE  { $$.val = true }
| GROUP { $$.val = true }

I suspect we should keep the SKIP DOC comment here. It probably affects whether we generate documentation.

---

pkg/sql/roleoption/role_option.go, line 150 at r2 (raw file):

}

func (ro RoleOption) toSQLValue() (string, error) {

unused?

---

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

// OptionWithValue contains a RoleOption and it's value as an Expr.
type OptionWithValue struct {

We should probably call this RoleOptionWithValue if it's role-specific.

I also noticed that there is already a KVOption type which serves a similar purpose for other commands (backup, restore, import). This includes validation via the TypeAsStringOpts function. Could you look into whether we could reuse that here rather than defining our own?

---

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

// Format prints out the list in a buffer.
// This keeps the existing order and uses " " as separator.
func (rol OptionsWithValues) Format(ctx *FmtCtx) {

Nit: rol is kind of a weird name here. More standard to call it o or ov. Same goes for the other functions.

---

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

// CreateUserOrRole represents a CREATE USER or CREATE ROLE statement.
type CreateUserOrRole struct {

Like in sql.y, I'm wondering if it would be clearer to keep CreateUser and CreateRole separate here, even if it requires some slight code duplication.

---

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

// AlterUserOrRoleOptions represents an ALTER ROLE or ALTER USER statement.
type AlterUserOrRoleOptions struct {

How about just AlterRole?

---

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

// DropUserOrRole represents a DROP USER statement
type DropUserOrRole struct {

DropRole?

---

pkg/sql/alter_user.go, line 96 at r1 (raw file):

		return err
	}
	if n.run.rowsAffected == 0 && !n.ifExists {

Do we still need the ifExists logic?

---

pkg/sql/alter_user_or_role.go, line 26 at r2 (raw file):

// alterUserOrRoleNode represents an ALTER ROLE ... [WITH] OPTION... statement.
type alterUserOrRoleNode struct {

I would say let's just call this alterRoleNode now since ALTER USER will just be an alias for it.

[RichardJCai]

For the first question, I think keeping them separate or merging them will depend on how we choose to handle some of the enterprise/non-enterprise stuff. If we choose to fully merge it, I think it would make sense to merge the syntax as they would function the same.

I'll look into using KVOption instead of OptionWithValue.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai and @solongordon)

---

docs/generated/settings/settings.html, line at r2 (raw file):

Previously, solongordon (Solon) wrote…

Looks like this file went away for some reason?

Done.

---

pkg/ccl/logictestccl/testdata/logic_test/role, line 53 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Maybe we should still have a test for this if we intend for roles to be droppable with DROP USER?

Yeah I was thinking about merging the tests for roles and users together once I get some more clarification on how to handle the separation between them. (For enterprise)

If we choose to fully match pg, then a lot of the role/user separated stuff will be completely merged.

Added test back.

---

pkg/ccl/roleccl/role.go, line 116 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Should we just change the function name to GetAllRoles? And does it still need to return whether each is a user or a role?

Updated map to behave as a set. All values of the map are true. Used to check for existence of usernames.

---

pkg/ccl/roleccl/role.go, line 125 at r2 (raw file):

Previously, solongordon (Solon) wrote…

You can just update the comment.

Done.

---

pkg/sql/alter_user.go, line 96 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Do we still need the ifExists logic?

Added back

---

pkg/sql/database.go, line 237 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Bad rename

Done.

---

pkg/sql/parser/help_test.go, line 69 at r2 (raw file):

Previously, solongordon (Solon) wrote…

This looks weird. If anything I would expect to show the ALTER ROLE help for both ALTER ROLE and ALTER USER. Though it might be nicer to keep them separate.

Will keep them separate since we're still not allowing non-enterprise to use the CREATE ROLE syntax.

---

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

Previously, solongordon (Solon) wrote…

I still think it's fine if we just insert the WITH when it's not specified. shrug

Updated to insert the WITH when it;'s not specified.

---

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

Previously, solongordon (Solon) wrote…

I don't think you need to add this. You can just use the existing bool() method.

Done.

---

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

Previously, solongordon (Solon) wrote…

It's unclear to me whether we should be merging these statements at the syntax level. CREATE USER is still slightly different from CREATE ROLE and we probably want to show different help text for the two. Do you think perhaps we should keep them separate here even though the underlying implementation is going to be the same?

Yeah, going to split them mainly because to show different help text. Also because non-enterprise still won't have access to CREATE ROLE syntax

---

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

Previously, solongordon (Solon) wrote…

I think you should be able to avoid having separate cases for no options, role_options, and WITH role_options. Check out how opt_with_options is defined.

Good call, that really simplifies things.

---

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

Previously, solongordon (Solon) wrote…

I suspect we should keep the SKIP DOC comment here. It probably affects whether we generate documentation.

Done.

---

pkg/sql/roleoption/role_option.go, line 150 at r2 (raw file):

Previously, solongordon (Solon) wrote…

unused?

Removed.

---

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

Previously, solongordon (Solon) wrote…

We should probably call this RoleOptionWithValue if it's role-specific.

I also noticed that there is already a KVOption type which serves a similar purpose for other commands (backup, restore, import). This includes validation via the TypeAsStringOpts function. Could you look into whether we could reuse that here rather than defining our own?

KVOptions seems to work.

---

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

Previously, solongordon (Solon) wrote…

Like in sql.y, I'm wondering if it would be clearer to keep CreateUser and CreateRole separate here, even if it requires some slight code duplication.

Yeah I think keeping them separate is a good idea as long as we keep the CreateRole version disabled for non-enterprise since it makes it easier to hide the CreateRole version behind the CCL license.

I went ahead and duplicated them.

---

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

Previously, solongordon (Solon) wrote…

How about just AlterRole?

Done.

---

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

Previously, solongordon (Solon) wrote…

DropRole?

Done.

---

pkg/sql/alter_user_or_role.go, line 26 at r2 (raw file):

Previously, solongordon (Solon) wrote…

I would say let's just call this alterRoleNode now since ALTER USER will just be an alias for it.

Done.

[jordanlewis]

This looks good to me besides the commit message, which needs to be updated since the functionality isn't added in this PR. I also have a couple of small nits.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @otan, @RichardJCai, and @solongordon)

---

pkg/ccl/roleccl/role.go, line 134 at r13 (raw file):

		}

		// Check that roles and roles exist.

This comment needs fixing

---

pkg/ccl/roleccl/role.go, line 286 at r13 (raw file):

		}

		// Check that roles and roles exist.

comment needs updating

---

pkg/sql/alter_role.go, line 82 at r13 (raw file):

// alterRoleRun is the run-time state of
// AlterRoleNode for local execution.

nit: the struct is still called alterRoleNode no capitalization.

---

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

// Format implements the NodeFormatter interface.
func (node *AlterUser) Format(ctx *FmtCtx) {
	// WIP: Need to fix this.

What needs to be fixed here?

[RichardJCai]

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

---

pkg/ccl/roleccl/role.go, line 134 at r13 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This comment needs fixing

Done.

---

pkg/ccl/roleccl/role.go, line 286 at r13 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

comment needs updating

Done

---

pkg/sql/alter_role.go, line 82 at r13 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: the struct is still called alterRoleNode no capitalization.

Fixed

---

pkg/sql/backfill.go, line 834 at r2 (raw file):

Previously, solongordon (Solon) wrote…

Bad rename? Many more examples of this in other files too. I won't comment on them all.

Done.

---

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

Previously, jordanlewis (Jordan Lewis) wrote…

What needs to be fixed here?

Oops, forgot to remove this comment.

[jordanlewis]

I think the release note for this is still wrong.

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

[jordanlewis]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @knz, @otan, and @solongordon)

[RichardJCai]

Thanks for the review!
bors r=jordanlewis

[craig]

Build succeeded

• GitHub CI (Cockroach)