Merge #46819

46819: release-20.1: sql: Change output of CREATE/ALTER/DROP ROLE to not include rows affected. Add hint for GRANT ROLE. r=knz a=RichardJCai Backport 1/1 commits from #46795. /cc @cockroachdb/release Co-authored-by: Richard Cai <[email protected]>
cockroachdb · Apr 7, 2020 · b47493f · b47493f
2 parents a84b18f + d12c727
commit b47493f
Show file tree

Hide file tree

Showing 7 changed files with 25 additions and 58 deletions.
diff --git a/pkg/cli/cli_test.go b/pkg/cli/cli_test.go
@@ -472,7 +472,7 @@ func Example_demo() {
 	//   system
 	// (4 rows)
 	// demo --insecure=false -e CREATE USER test WITH PASSWORD 'testpass'
-	// CREATE ROLE 1
+	// CREATE ROLE
 	// demo -e CREATE USER test WITH PASSWORD 'testpass'
 	// ERROR: setting or updating a password is not supported in insecure mode
 	// SQLSTATE: 28P01

diff --git a/pkg/sql/alter_role.go b/pkg/sql/alter_role.go
@@ -31,8 +31,6 @@ type alterRoleNode struct {
 	ifExists    bool
 	isRole      bool
 	roleOptions roleoption.List
-
-	run alterRoleRun
 }
 
 // AlterRole represents a ALTER ROLE statement.
@@ -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 {
@@ -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,
@@ -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,
@@ -204,7 +196,6 @@ func (n *alterRoleNode) startExec(params runParams) error {
 		if err != nil {
 			return err
 		}
-		n.run.rowsAffected += rowsAffected
 	}
 
 	return nil
@@ -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
-}
diff --git a/pkg/sql/create_role.go b/pkg/sql/create_role.go
@@ -32,8 +32,6 @@ type CreateRoleNode struct {
 	isRole      bool
 	roleOptions roleoption.List
 	userNameInfo
-
-	run createUserRun
 }
 
 var userTableName = tree.NewTableName("system", "users")
@@ -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,
@@ -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,
 		)
 	}
 
@@ -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,
@@ -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 }
 
@@ -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."
 

diff --git a/pkg/sql/drop_role.go b/pkg/sql/drop_role.go
@@ -31,8 +31,6 @@ type DropRoleNode struct {
 	ifExists bool
 	isRole   bool
 	names    func() ([]string, error)
-
-	run dropRoleRun
 }
 
 // DropRole represents a DROP ROLE statement.
@@ -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 {
@@ -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.
@@ -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,
@@ -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,
@@ -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,
@@ -214,8 +205,6 @@ func (n *DropRoleNode) startExec(params runParams) error {
 		if err != nil {
 			return err
 		}
-
-		numRoleMembershipsDeleted += rowsAffected
 	}
 
 	if numRoleMembershipsDeleted > 0 {
@@ -226,9 +215,6 @@ func (n *DropRoleNode) startExec(params runParams) error {
 		}
 	}
 
-	n.run.numDeleted = numUsersDeleted
-	n.run.numDeleted += numRoleOptionsDeleted
-
 	return nil
 }
 
@@ -240,6 +226,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 }
diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go
@@ -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.
@@ -85,6 +88,13 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 
 	for _, r := range n.Roles {
 		if _, ok := roles[string(r)]; !ok {
+			for name := range roleoption.ByName {
+				if uppercase := strings.ToUpper(string(r)); uppercase == 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.", uppercase)
+				}
+			}
 			return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r)
 		}
 	}

diff --git a/pkg/sql/plan.go b/pkg/sql/plan.go
@@ -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{}

diff --git a/pkg/sql/sem/tree/stmt.go b/pkg/sql/sem/tree/stmt.go
@@ -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" }
@@ -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" }
@@ -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" }