Skip to content

Commit

Permalink
Change output of CREATE/ALTER/DROP ROLE to not include rows affected.…
Browse files Browse the repository at this point in the history 
… Add hint for GRANT ROLE.

Fixes cockroachdb#46728, cockroachdb#46698

Release note (sql change): Remove rows affected from CREATE/ALTER/DROP ROLE
command results.
The number doesn't make sense to include since rows affected includes
system.role_options rows, this number can be misleading (ie CREATE ROLE returning 3).
This also matches PG as no number is returned for these commands.

Release note (sql change): Add hint to use ALTER ROLE when trying to
"GRANT" a role option directly to a user (using the GRANT ROLE syntax).

Release justification: Low risk change, command result output changed for
CREATE/ALTER/DROP ROLE. Hint added for GRANT ROLE.
  • Loading branch information
@RichardJCai
RichardJCai committed Mar 31, 2020
1 parent 1225203 commit 53de9b9
Show file tree
Hide file tree
Showing 6 changed files with 26 additions and 56 deletions.
17 changes: 2 additions & 15 deletions pkg/sql/alter_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ type alterRoleNode struct {
ifExists bool
isRole bool
roleOptions roleoption.List

run alterRoleRun
}

// AlterRole represents a ALTER ROLE statement.
Expand Down Expand Up @@ -77,12 +75,6 @@ func (p *planner) AlterRoleNode(
}, nil
}

// alterRoleRun is the run-time state of
// alterRoleNode for local execution.
type alterRoleRun struct {
rowsAffected int
}

func (n *alterRoleNode) startExec(params runParams) error {
var opName string
if n.isRole {
Expand Down Expand Up @@ -156,7 +148,7 @@ func (n *alterRoleNode) startExec(params runParams) error {

// Updating PASSWORD is a special case since PASSWORD lives in system.users
// while the rest of the role options lives in system.role_options.
n.run.rowsAffected, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
_, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
params.ctx,
opName,
params.p.txn,
Expand Down Expand Up @@ -193,7 +185,7 @@ func (n *alterRoleNode) startExec(params runParams) error {
}
}

rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
_, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
params.ctx,
opName,
params.p.txn,
Expand All @@ -204,7 +196,6 @@ func (n *alterRoleNode) startExec(params runParams) error {
if err != nil {
return err
}
n.run.rowsAffected += rowsAffected
}

return nil
Expand All @@ -213,7 +204,3 @@ func (n *alterRoleNode) startExec(params runParams) error {
func (*alterRoleNode) Next(runParams) (bool, error) { return false, nil }
func (*alterRoleNode) Values() tree.Datums { return tree.Datums{} }
func (*alterRoleNode) Close(context.Context) {}

func (n *alterRoleNode) FastPathResults() (int, bool) {
return n.run.rowsAffected, true
}
18 changes: 4 additions & 14 deletions pkg/sql/create_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ type CreateRoleNode struct {
isRole bool
roleOptions roleoption.List
userNameInfo

run createUserRun
}

var userTableName = tree.NewTableName("system", "users")
Expand Down Expand Up @@ -155,7 +153,7 @@ func (n *CreateRoleNode) startExec(params runParams) error {
}

// TODO(richardjcai): move hashedPassword column to system.role_options.
n.run.rowsAffected, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
params.ctx,
opName,
params.p.txn,
Expand All @@ -167,9 +165,9 @@ func (n *CreateRoleNode) startExec(params runParams) error {

if err != nil {
return err
} else if n.run.rowsAffected != 1 {
} else if rowsAffected != 1 {
return errors.AssertionFailedf("%d rows affected by user creation; expected exactly one row affected",
n.run.rowsAffected,
rowsAffected,
)
}

Expand Down Expand Up @@ -197,7 +195,7 @@ func (n *CreateRoleNode) startExec(params runParams) error {
}
}

rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
_, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.ExecEx(
params.ctx,
opName,
params.p.txn,
Expand All @@ -208,16 +206,11 @@ func (n *CreateRoleNode) startExec(params runParams) error {
if err != nil {
return err
}
n.run.rowsAffected += rowsAffected
}

return nil
}

type createUserRun struct {
rowsAffected int
}

// Next implements the planNode interface.
func (*CreateRoleNode) Next(runParams) (bool, error) { return false, nil }

Expand All @@ -227,9 +220,6 @@ func (*CreateRoleNode) Values() tree.Datums { return tree.Datums{} }
// Close implements the planNode interface.
func (*CreateRoleNode) Close(context.Context) {}

// FastPathResults implements the planNodeFastPath interface.
func (n *CreateRoleNode) FastPathResults() (int, bool) { return n.run.rowsAffected, true }

const usernameHelp = "Usernames are case insensitive, must start with a letter, " +
"digit or underscore, may contain letters, digits, dashes, periods, or underscores, and must not exceed 63 characters."

Expand Down
26 changes: 5 additions & 21 deletions pkg/sql/drop_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,6 @@ type DropRoleNode struct {
ifExists bool
isRole bool
names func() ([]string, error)

run dropRoleRun
}

// DropRole represents a DROP ROLE statement.
Expand Down Expand Up @@ -61,12 +59,6 @@ func (p *planner) DropRoleNode(
}, nil
}

// dropRoleRun contains the run-time state of DropRoleNode during local execution.
type dropRoleRun struct {
// The number of users deleted.
numDeleted int
}

func (n *DropRoleNode) startExec(params runParams) error {
var opName string
if n.isRole {
Expand Down Expand Up @@ -160,7 +152,7 @@ func (n *DropRoleNode) startExec(params runParams) error {
}

// All safe - do the work.
var numUsersDeleted, numRoleMembershipsDeleted, numRoleOptionsDeleted int
var numRoleMembershipsDeleted int
for normalizedUsername := range userNames {
// Specifically reject special users and roles. Some (root, admin) would fail with
// "privileges still exist" first.
Expand All @@ -173,7 +165,7 @@ func (n *DropRoleNode) startExec(params runParams) error {
pgcode.InvalidParameterValue, "cannot drop special user %s", normalizedUsername)
}

rowsAffected, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
numUsersDeleted, err := params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
params.ctx,
opName,
params.p.txn,
Expand All @@ -184,13 +176,12 @@ func (n *DropRoleNode) startExec(params runParams) error {
return err
}

if rowsAffected == 0 && !n.ifExists {
if numUsersDeleted == 0 && !n.ifExists {
return errors.Errorf("role/user %s does not exist", normalizedUsername)
}
numUsersDeleted += rowsAffected

// Drop all role memberships involving the user/role.
rowsAffected, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
numRoleMembershipsDeleted, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
params.ctx,
"drop-role-membership",
params.p.txn,
Expand All @@ -201,7 +192,7 @@ func (n *DropRoleNode) startExec(params runParams) error {
return err
}

numRoleOptionsDeleted, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
_, err = params.extendedEvalCtx.ExecCfg.InternalExecutor.Exec(
params.ctx,
opName,
params.p.txn,
Expand All @@ -214,8 +205,6 @@ func (n *DropRoleNode) startExec(params runParams) error {
if err != nil {
return err
}

numRoleMembershipsDeleted += rowsAffected
}

if numRoleMembershipsDeleted > 0 {
Expand All @@ -226,9 +215,6 @@ func (n *DropRoleNode) startExec(params runParams) error {
}
}

n.run.numDeleted = numUsersDeleted
n.run.numDeleted += numRoleOptionsDeleted

return nil
}

Expand All @@ -241,5 +227,3 @@ func (*DropRoleNode) Values() tree.Datums { return tree.Datums{} }
// Close implements the planNode interface.
func (*DropRoleNode) Close(context.Context) {}

// FastPathResults implements the planNodeFastPath interface.
func (n *DropRoleNode) FastPathResults() (int, bool) { return n.run.numDeleted, true }
12 changes: 12 additions & 0 deletions pkg/sql/grant_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,17 @@ package sql

import (
"context"
"strings"

"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
"github.com/cockroachdb/cockroach/pkg/util/tracing"
"github.com/cockroachdb/errors"
)

// GrantRoleNode creates entries in the system.role_members table.
Expand Down Expand Up @@ -83,8 +86,17 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
// NOTE: membership manipulation involving the "public" pseudo-role fails with
// "role public does not exist". This matches postgres behavior.


for _, r := range n.Roles {
if _, ok := roles[string(r)]; !ok {
for name := range roleoption.ByName {
if strings.ToUpper(string(r)) == name {
return nil,
errors.WithHintf(
pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r),
"%s is a role option, try using ALTER ROLE to change a role's options.", strings.ToUpper(string(r)))
}
}
return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r)
}
}
Expand Down
3 changes: 0 additions & 3 deletions pkg/sql/plan.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,6 @@ var _ planNode = &virtualTableNode{}
var _ planNode = &windowNode{}
var _ planNode = &zeroNode{}

var _ planNodeFastPath = &CreateRoleNode{}
var _ planNodeFastPath = &DropRoleNode{}
var _ planNodeFastPath = &alterRoleNode{}
var _ planNodeFastPath = &deleteRangeNode{}
var _ planNodeFastPath = &rowCountNode{}
var _ planNodeFastPath = &serializeNode{}
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/sem/tree/stmt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ func (*AlterSequence) StatementType() StatementType { return DDL }
func (*AlterSequence) StatementTag() string { return "ALTER SEQUENCE" }

// StatementType implements the Statement interface.
func (*AlterRole) StatementType() StatementType { return RowsAffected }
func (*AlterRole) StatementType() StatementType { return Ack }

// StatementTag returns a short string identifying the type of statement.
func (*AlterRole) StatementTag() string { return "ALTER ROLE" }
Expand Down Expand Up @@ -333,7 +333,7 @@ func (n *CreateTable) StatementTag() string {
func (*CreateTable) modifiesSchema() bool { return true }

// StatementType implements the Statement interface.
func (*CreateRole) StatementType() StatementType { return RowsAffected }
func (*CreateRole) StatementType() StatementType { return Ack }

// StatementTag returns a short string identifying the type of statement.
func (*CreateRole) StatementTag() string { return "CREATE ROLE" }
Expand Down Expand Up @@ -415,7 +415,7 @@ func (*DropSequence) StatementType() StatementType { return DDL }
func (*DropSequence) StatementTag() string { return "DROP SEQUENCE" }

// StatementType implements the Statement interface.
func (*DropRole) StatementType() StatementType { return RowsAffected }
func (*DropRole) StatementType() StatementType { return Ack }

// StatementTag returns a short string identifying the type of statement.
func (*DropRole) StatementTag() string { return "DROP ROLE" }
Expand Down

0 comments on commit 53de9b9

Please sign in to comment.