Merge #74172 #74596

74172: sql: refactor pg_builtin to use actual grant options r=rafiss a=ecwall

refs #73129

The builtins has_table_privilege, has_column_privilege,
has_any_column_privilege now use privileges.Priv.GrantOption instead 
of privileges.Kind.GRANT.

Refactor builtins.priv -> privilege.Privilege.
    
Replace privilege.Kind with privilege.Privilege in functions that need
access to privilege.Privilege.GrantOption.


74596: dev: introduce common benchmarking flags r=irfansharif a=irfansharif

Specifically:
  --bench-mem (to print out memory allocations);
  --bench-time (to control how long each benchmark is run for);
  --count (how many times to run it, also added to `dev test`);
  -v and --show-logs (similar to `dev test`)

We also add supports for args-after-double-dash-are-for-bazel within
`dev bench`. This commit is light on testing (read: there isn't any), so
doesn't bump DEV_VERSION to roll it out to everyone just yet.

Release note: None

Co-authored-by: Evan Wall <wall@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>

pkg/ccl/importccl/import_table_creation.go

@@ -333,7 +333,7 @@ func (so *importSequenceOperators) HasPrivilege(
 	ctx context.Context,
 	specifier tree.HasPrivilegeSpecifier,
 	user security.SQLUsername,
-	kind privilege.Kind,
+	priv privilege.Privilege,
 ) (bool, error) {
 	return false, errors.WithStack(errSequenceOperators)
 }

pkg/cmd/dev/bench.go

@@ -19,27 +19,51 @@ import (
 	"github.com/spf13/cobra"
 )
 
+const (
+	benchTimeFlag = "bench-time"
+	benchMemFlag  = "bench-mem"
+)
+
 // makeBenchCmd constructs the subcommand used to run the specified benchmarks.
 func makeBenchCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Command {
 	benchCmd := &cobra.Command{
 		Use:   "bench [pkg...]",
 		Short: `Run the specified benchmarks`,
 		Long:  `Run the specified benchmarks.`,
 		Example: `
-	dev bench pkg/sql/parser --filter=BenchmarkParse`,
+	dev bench pkg/sql/parser --filter=BenchmarkParse
+	dev bench pkg/bench -f='BenchmarkTracing/1node/scan/trace=off' --count=2 --bench-time=10x --bench-mem`,
 		Args: cobra.MinimumNArgs(0),
 		RunE: runE,
 	}
 	addCommonBuildFlags(benchCmd)
 	addCommonTestFlags(benchCmd)
+
+	benchCmd.Flags().BoolP(vFlag, "v", false, "show benchmark process output")
+	benchCmd.Flags().BoolP(showLogsFlag, "", false, "show crdb logs in-line")
+	benchCmd.Flags().Int(countFlag, 1, "run benchmark n times")
+	// We use a string flag for benchtime instead of a duration; the go test
+	// runner accepts input of the form "Nx" to run the benchmark N times (see
+	// `go help testflag`).
+	benchCmd.Flags().String(benchTimeFlag, "", "duration to run each benchmark for")
+	benchCmd.Flags().Bool(benchMemFlag, false, "print memory allocations for benchmarks")
+
 	return benchCmd
 }
 
-func (d *dev) bench(cmd *cobra.Command, pkgs []string) error {
+func (d *dev) bench(cmd *cobra.Command, commandLine []string) error {
+	pkgs, additionalBazelArgs := splitArgsAtDash(cmd, commandLine)
 	ctx := cmd.Context()
-	filter := mustGetFlagString(cmd, filterFlag)
-	timeout := mustGetFlagDuration(cmd, timeoutFlag)
-	short := mustGetFlagBool(cmd, shortFlag)
+	var (
+		filter    = mustGetFlagString(cmd, filterFlag)
+		timeout   = mustGetFlagDuration(cmd, timeoutFlag)
+		short     = mustGetFlagBool(cmd, shortFlag)
+		showLogs  = mustGetFlagBool(cmd, showLogsFlag)
+		verbose   = mustGetFlagBool(cmd, vFlag)
+		count     = mustGetFlagInt(cmd, countFlag)
+		benchTime = mustGetFlagString(cmd, benchTimeFlag)
+		benchMem  = mustGetFlagBool(cmd, benchMemFlag)
+	)
 
 	// Enumerate all benches to run.
 	if len(pkgs) == 0 {
@@ -87,13 +111,30 @@ func (d *dev) bench(cmd *cobra.Command, pkgs []string) error {
 	if numCPUs != 0 {
 		argsBase = append(argsBase, fmt.Sprintf("--local_cpu_resources=%d", numCPUs))
 	}
+	if verbose {
+		argsBase = append(argsBase, "--test_arg", "-test.v")
+	}
+	if showLogs {
+		argsBase = append(argsBase, "--test_arg", "-show-logs")
+	}
+	if count != 1 {
+		argsBase = append(argsBase, "--test_arg", fmt.Sprintf("-test.count=%d", count))
+	}
+	if benchTime != "" {
+		argsBase = append(argsBase, "--test_arg", fmt.Sprintf("-test.benchtime=%s", benchTime))
+	}
+	if benchMem {
+		argsBase = append(argsBase, "--test_arg", "-test.benchmem")
+	}
 
 	for _, bench := range benches {
 		args := make([]string, len(argsBase))
 		copy(args, argsBase)
 		base := filepath.Base(bench)
 		target := "//" + bench + ":" + base + "_test"
-		args = append(args, target, "--", "-test.run=-")
+		args = append(args, target)
+		args = append(args, additionalBazelArgs...)
+		args = append(args, "--", "-test.run=-")
 		if filter == "" {
 			args = append(args, "-test.bench=.")
 		} else {

pkg/cmd/dev/build.go

@@ -87,6 +87,7 @@ func makeBuildCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Com
 // TODO(irfansharif): Make sure all the relevant binary targets are defined
 // above, and in usage docs.
 
+// buildTargetMapping maintains shorthands that map 1:1 with bazel targets.
 var buildTargetMapping = map[string]string{
 	"buildifier":       "@com_github_bazelbuild_buildtools//buildifier:buildifier",
 	"buildozer":        "@com_github_bazelbuild_buildtools//buildozer:buildozer",
@@ -255,7 +256,7 @@ func targetToBinBasename(target string) string {
 // (e.g. after translation, so short -> "//pkg/cmd/cockroach-short").
 func (d *dev) getBasicBuildArgs(
 	ctx context.Context, targets []string, skipGenerate bool,
-) (args []string, buildTargets []buildTarget, err error) {
+) (args []string, buildTargets []buildTarget, _ error) {
 	if len(targets) == 0 {
 		// Default to building the cockroach binary.
 		targets = append(targets, "cockroach")
@@ -277,8 +278,8 @@ func (d *dev) getBasicBuildArgs(
 			queryArgs := []string{"query", target, "--output=label_kind"}
 			labelKind, queryErr := d.exec.CommandContextSilent(ctx, "bazel", queryArgs...)
 			if queryErr != nil {
-				err = fmt.Errorf("could not run `bazel %s` (%w)", shellescape.QuoteCommand(queryArgs), queryErr)
-				return
+				return nil, nil, fmt.Errorf("could not run `bazel %s` (%w)",
+					shellescape.QuoteCommand(queryArgs), queryErr)
 			}
 			for _, line := range strings.Split(strings.TrimSpace(string(labelKind)), "\n") {
 				fields := strings.Fields(line)
@@ -299,10 +300,10 @@ func (d *dev) getBasicBuildArgs(
 			}
 			continue
 		}
+
 		aliased, ok := buildTargetMapping[target]
 		if !ok {
-			err = fmt.Errorf("unrecognized target: %s", target)
-			return
+			return nil, nil, fmt.Errorf("unrecognized target: %s", target)
 		}
 
 		args = append(args, aliased)
@@ -324,7 +325,7 @@ func (d *dev) getBasicBuildArgs(
 		args = append(args, "--config=test")
 	}
 
-	return
+	return args, buildTargets, nil
 }
 
 // Hoist generated code out of the sandbox and into the workspace.

pkg/cmd/dev/test.go

@@ -27,6 +27,7 @@ const (
 	stressTarget = "@com_github_cockroachdb_stress//:stress"
 
 	// General testing flags.
+	countFlag       = "count"
 	vFlag           = "verbose"
 	showLogsFlag    = "show-logs"
 	stressFlag      = "stress"
@@ -35,6 +36,7 @@ const (
 	ignoreCacheFlag = "ignore-cache"
 	rewriteFlag     = "rewrite"
 	rewriteArgFlag  = "rewrite-arg"
+	vModuleFlag     = "vmodule"
 )
 
 func makeTestCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Command {
@@ -46,6 +48,7 @@ func makeTestCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Comm
 		Example: `
 	dev test
 	dev test pkg/kv/kvserver --filter=TestReplicaGC* -v --timeout=1m
+	dev test pkg/server -f=TestSpanStatsResponse -v --count=5 --vmodule='raft=1'
 	dev test --stress --race ...`,
 		Args: cobra.MinimumNArgs(0),
 		RunE: runE,
@@ -63,14 +66,16 @@ func makeTestCmd(runE func(cmd *cobra.Command, args []string) error) *cobra.Comm
 	// under test, controlling whether the process-internal logs are made
 	// visible.
 	testCmd.Flags().BoolP(vFlag, "v", false, "show testing process output")
+	testCmd.Flags().Int(countFlag, 1, "run test the given number of times")
 	testCmd.Flags().BoolP(showLogsFlag, "", false, "show crdb logs in-line")
 	testCmd.Flags().Bool(stressFlag, false, "run tests under stress")
 	testCmd.Flags().String(stressArgsFlag, "", "additional arguments to pass to stress")
 	testCmd.Flags().Bool(raceFlag, false, "run tests using race builds")
 	testCmd.Flags().Bool(ignoreCacheFlag, false, "ignore cached test runs")
-	testCmd.Flags().String(rewriteFlag, "", "argument to pass to underlying (only applicable for certain tests, e.g. logic and datadriven tests). If unspecified, -rewrite will be passed to the test binary.")
+	testCmd.Flags().String(rewriteFlag, "", "argument to pass to underlying test binary (only applicable to certain tests)")
 	testCmd.Flags().String(rewriteArgFlag, "", "additional argument to pass to -rewrite (implies --rewrite)")
 	testCmd.Flags().Lookup(rewriteFlag).NoOptDefVal = "-rewrite"
+	testCmd.Flags().String(vModuleFlag, "", "comma-separated list of pattern=N settings for file-filtered logging")
 	return testCmd
 }
 
@@ -89,6 +94,8 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error {
 		timeout     = mustGetFlagDuration(cmd, timeoutFlag)
 		verbose     = mustGetFlagBool(cmd, vFlag)
 		showLogs    = mustGetFlagBool(cmd, showLogsFlag)
+		count       = mustGetFlagInt(cmd, countFlag)
+		vModule     = mustGetFlagString(cmd, vModuleFlag)
 	)
 
 	var args []string
@@ -227,6 +234,15 @@ func (d *dev) test(cmd *cobra.Command, commandLine []string) error {
 	if showLogs {
 		args = append(args, "--test_arg", "-show-logs")
 	}
+	if count != 1 {
+		args = append(args, "--test_arg", fmt.Sprintf("-test.count=%d", count))
+	}
+	if vModule != "" {
+		args = append(args, "--test_arg", fmt.Sprintf("-vmodule=%s", vModule))
+	}
+	// TODO(irfansharif): Support --go-flags, to pass in an arbitrary set of
+	// flags into the go test binaries. Gives better coverage to everything
+	// listed under `go help testflags`.
 
 	{ // Handle test output flags.
 		testOutputArgs := []string{"--test_output", "errors"}

pkg/cmd/dev/util.go

@@ -57,6 +57,14 @@ func mustGetFlagBool(cmd *cobra.Command, name string) bool {
 	return val
 }
 
+func mustGetFlagInt(cmd *cobra.Command, name string) int {
+	val, err := cmd.Flags().GetInt(name)
+	if err != nil {
+		log.Fatalf("unexpected error: %v", err)
+	}
+	return val
+}
+
 func mustGetFlagDuration(cmd *cobra.Command, name string) time.Duration {
 	val, err := cmd.Flags().GetDuration(name)
 	if err != nil {

pkg/sql/faketreeeval/evalctx.go

@@ -85,7 +85,7 @@ func (so *DummySequenceOperators) HasPrivilege(
 	ctx context.Context,
 	specifier tree.HasPrivilegeSpecifier,
 	user security.SQLUsername,
-	kind privilege.Kind,
+	priv privilege.Privilege,
 ) (bool, error) {
 	return false, errors.WithStack(errEvalPlanner)
 }
@@ -305,7 +305,7 @@ func (ep *DummyEvalPlanner) HasPrivilege(
 	ctx context.Context,
 	specifier tree.HasPrivilegeSpecifier,
 	user security.SQLUsername,
-	kind privilege.Kind,
+	priv privilege.Privilege,
 ) (bool, error) {
 	return false, errors.WithStack(errEvalPlanner)
 }

pkg/sql/privilege/privilege.go

@@ -45,6 +45,17 @@ const (
 	RULE       Kind = 12
 )
 
+// Privilege represents a privilege parsed from an Access Privilege Inquiry
+// Function's privilege string argument.
+type Privilege struct {
+	Kind Kind
+	// Each privilege Kind has an optional "grant option" flag associated with
+	// it. A role can only grant a privilege on an object to others if it is the
+	// owner of the object or if it itself holds that privilege WITH GRANT OPTION
+	// on the object. This replaces the CockroachDB-specific GRANT privilege.
+	GrantOption bool
+}
+
 // ObjectType represents objects that can have privileges.
 type ObjectType string
 

pkg/sql/resolver.go

@@ -304,7 +304,7 @@ func (p *planner) HasPrivilege(
 	ctx context.Context,
 	specifier tree.HasPrivilegeSpecifier,
 	user security.SQLUsername,
-	kind privilege.Kind,
+	priv privilege.Privilege,
 ) (bool, error) {
 	desc, err := p.ResolveDescriptorForPrivilegeSpecifier(
 		ctx,
@@ -315,32 +315,42 @@ func (p *planner) HasPrivilege(
 	}
 
 	// hasPrivilegeFunc checks whether any role has the given privilege.
-	hasPrivilegeFunc := func(priv privilege.Kind) (bool, error) {
-		err := p.CheckPrivilegeForUser(ctx, desc, priv, user)
+	hasPrivilegeFunc := func(priv privilege.Privilege) (bool, error) {
+		err := p.CheckPrivilegeForUser(ctx, desc, priv.Kind, user)
+		if err == nil {
+			if priv.GrantOption {
+				if !p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.ValidateGrantOption) {
+					err = p.CheckPrivilegeForUser(ctx, desc, privilege.GRANT, user)
+				} else {
+					err = p.CheckGrantOptionsForUser(ctx, desc, []privilege.Kind{priv.Kind}, true /* isGrant */)
+				}
+			}
+		}
 		if err != nil {
 			if pgerror.GetPGCode(err) == pgcode.InsufficientPrivilege {
 				return false, nil
 			}
 			return false, err
 		}
+
 		return true, nil
 	}
 
-	if kind == privilege.RULE {
+	if priv.Kind == privilege.RULE {
 		// RULE was only added for compatibility with Postgres, and Postgres
 		// never allows RULE to be granted, even if the user has ALL privileges.
 		// See https://www.postgresql.org/docs/8.1/sql-grant.html
 		// and https://www.postgresql.org/docs/release/8.2.0/.
 		return false, nil
 	}
-	hasPrivilege, err := hasPrivilegeFunc(privilege.ALL)
+	hasPrivilege, err := hasPrivilegeFunc(privilege.Privilege{Kind: privilege.ALL})
 	if err != nil {
 		return false, err
 	}
 	if hasPrivilege {
 		return true, nil
 	}
-	return hasPrivilegeFunc(kind)
+	return hasPrivilegeFunc(priv)
 }
 
 // ResolveDescriptorForPrivilegeSpecifier resolves a tree.HasPrivilegeSpecifier