Merge #75854 #75892 #75901 #75910

75854: sql: update SHOW GRANTS ON DATABASE to include grant options r=RichardJCai a=ecwall

refs #73394

Release note (sql): SHOW GRANTS ON DATABASE includes is_grantable column

75892: distsql: cleanup log tag annotation r=andreimatei a=andreimatei

A context annotation was not doing what it wanted; it was inadvertently
a no-op.

This patch also makes FlowCtx.AmbientCtx not be embedded. Embedded
AmbientCtxs are annoying because one can't easily find where they're
used.

Release note: None

75901: distsql: improve flow cleanup r=andreimatei a=andreimatei

FlowBase.Cleanup(ctx) was closing the tracing span from ctx, if any.
This was assuming that the respective ctx is the one associated with the
flow, and that the flow always has a span. These are brittle
assumptions; in particular, I intend to avoid opening spans for local
flows when there's no particular reason to (i.e. when not collecting
stats), to save on their cost.

This patch has the Flow explicitly keep track of which Span it owns, if
any.

Release note: None

75910: migration/migrations: skip TestEnsureSpanConfigReconciliation r=irfansharif a=otan

Refs: #75849

Reason: flaky test

Generated by bin/skip-test.

Release justification: non-production code changes

Release note: None

Co-authored-by: Evan Wall <[email protected]>
Co-authored-by: Andrei Matei <[email protected]>
Co-authored-by: Oliver Tan <[email protected]>

pkg/ccl/backupccl/restore_old_versions_test.go

@@ -439,9 +439,9 @@ func restoreV201ZoneconfigPrivilegeTest(exportDir string) func(t *testing.T) {
 		require.NoError(t, err)
 		sqlDB.Exec(t, `RESTORE FROM $1`, LocalFoo)
 		testDBGrants := [][]string{
-			{"test", "admin", "ALL"},
-			{"test", "root", "ALL"},
-			{"test", "testuser", "ZONECONFIG"},
+			{"test", "admin", "ALL", "true"},
+			{"test", "root", "ALL", "true"},
+			{"test", "testuser", "ZONECONFIG", "false"},
 		}
 		sqlDB.CheckQueryResults(t, `show grants on database test`, testDBGrants)
 

pkg/ccl/backupccl/testdata/backup-restore/restore-grants

@@ -51,7 +51,7 @@ CREATE TABLE testdb.testtable_greeting_owner (a testdb.greeting_owner);
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR user1;
 ----
-testdb user1 ALL
+testdb user1 ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA public FOR user1;
@@ -76,7 +76,7 @@ SHOW GRANTS ON TABLE testdb.testtable_simple FOR user1;
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR testuser;
 ----
-testdb testuser ALL
+testdb testuser ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA public FOR testuser;
@@ -155,10 +155,10 @@ RESTORE testdb.sc.othertable, testdb.testtable_greeting_usage FROM 'nodelocal://
 query-sql
 SHOW GRANTS ON DATABASE testuser_db;
 ----
-testuser_db admin ALL
-testuser_db public CONNECT
-testuser_db root ALL
-testuser_db testuser CREATE
+testuser_db admin ALL true
+testuser_db public CONNECT false
+testuser_db root ALL true
+testuser_db testuser CREATE false
 
 query-sql
 SHOW GRANTS ON SCHEMA public;
@@ -296,7 +296,7 @@ SHOW GRANTS ON testdb.sc.othertable FOR testuser;
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR admin;
 ----
-testdb admin ALL
+testdb admin ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA testdb.public FOR admin;
@@ -387,7 +387,7 @@ SHOW GRANTS ON testdb.sc.othertable FOR testuser;
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR admin;
 ----
-testdb admin ALL
+testdb admin ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA testdb.public FOR admin;
@@ -433,7 +433,7 @@ RESTORE FROM 'nodelocal://0/test/';
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR user1;
 ----
-testdb user1 ALL
+testdb user1 ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA testdb.public FOR user1;
@@ -458,7 +458,7 @@ SHOW GRANTS ON TABLE testdb.testtable_simple FOR user1;
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR testuser;
 ----
-testdb testuser ALL
+testdb testuser ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA testdb.public FOR testuser;
@@ -496,7 +496,7 @@ ALTER TYPE testdb.greeting_owner ADD VALUE 'new' BEFORE 'howdy';
 query-sql
 SHOW GRANTS ON DATABASE testdb FOR admin;
 ----
-testdb admin ALL
+testdb admin ALL true
 
 query-sql
 SHOW GRANTS ON SCHEMA testdb.public FOR admin;

pkg/migration/migrations/migrate_span_configs_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/spanconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -34,6 +35,7 @@ import (
 // span config reconciliation attempt, blocking until it occurs.
 func TestEnsureSpanConfigReconciliation(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+	skip.WithIssue(t, 75849, "flaky test")
 	defer log.Scope(t).Close(t)
 
 	ctx := context.Background()

pkg/sql/crdb_internal.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/build"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/config/zonepb"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
@@ -4569,7 +4570,8 @@ var crdbInternalClusterDatabasePrivilegesTable = virtualSchemaTable{
 CREATE TABLE crdb_internal.cluster_database_privileges (
 	database_name   STRING NOT NULL,
 	grantee         STRING NOT NULL,
-	privilege_type  STRING NOT NULL
+	privilege_type  STRING NOT NULL,
+	is_grantable 		STRING
 )`,
 	populate: func(ctx context.Context, p *planner, dbContext catalog.DatabaseDescriptor, addRow func(...tree.Datum) error) error {
 		return forEachDatabaseDesc(ctx, p, dbContext, true, /* requiresPrivileges */
@@ -4578,13 +4580,21 @@ CREATE TABLE crdb_internal.cluster_database_privileges (
 				dbNameStr := tree.NewDString(db.GetName())
 				// TODO(knz): This should filter for the current user, see
 				// https://github.com/cockroachdb/cockroach/issues/35572
+				populateGrantOption := p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.ValidateGrantOption)
 				for _, u := range privs {
 					userNameStr := tree.NewDString(u.User.Normalized())
 					for _, priv := range u.Privileges {
+						var isGrantable tree.Datum
+						if populateGrantOption {
+							isGrantable = yesOrNoDatum(priv.GrantOption)
+						} else {
+							isGrantable = tree.DNull
+						}
 						if err := addRow(
 							dbNameStr,                           // database_name
 							userNameStr,                         // grantee
 							tree.NewDString(priv.Kind.String()), // privilege_type
+							isGrantable,                         // is_grantable
 						); err != nil {
 							return err
 						}

pkg/sql/delegate/show_grants.go

@@ -34,7 +34,8 @@ func (d *delegator) delegateShowGrants(n *tree.ShowGrants) (tree.Statement, erro
 	const dbPrivQuery = `
 SELECT database_name,
        grantee,
-       privilege_type
+       privilege_type,
+			 is_grantable::boolean
   FROM "".crdb_internal.cluster_database_privileges`
 	const schemaPrivQuery = `
 SELECT table_catalog AS database_name,

pkg/sql/distsql/server.go

@@ -376,7 +376,7 @@ func (ds *ServerImpl) setupFlow(
 	// to restore the original value which can have data races under stress.
 	isVectorized := req.EvalContext.SessionData.VectorizeMode != sessiondatapb.VectorizeOff
 	f := newFlow(
-		flowCtx, ds.flowRegistry, rowSyncFlowConsumer, batchSyncFlowConsumer,
+		flowCtx, sp, ds.flowRegistry, rowSyncFlowConsumer, batchSyncFlowConsumer,
 		localState.LocalProcs, isVectorized, onFlowCleanup, req.StatementSQL,
 	)
 	opt := flowinfra.FuseNormally
@@ -397,8 +397,8 @@ func (ds *ServerImpl) setupFlow(
 		return ctx, nil, nil, err
 	}
 	if !f.IsLocal() {
-		flowCtx.AddLogTag("f", f.GetFlowCtx().ID.Short())
-		flowCtx.AnnotateCtx(ctx)
+		flowCtx.AmbientContext.AddLogTag("f", f.GetFlowCtx().ID.Short())
+		ctx = flowCtx.AmbientContext.AnnotateCtx(ctx)
 		telemetry.Inc(sqltelemetry.DistSQLExecCounter)
 	}
 	if f.IsVectorized() {
@@ -493,6 +493,7 @@ func (ds *ServerImpl) newFlowContext(
 
 func newFlow(
 	flowCtx execinfra.FlowCtx,
+	sp *tracing.Span,
 	flowReg *flowinfra.FlowRegistry,
 	rowSyncFlowConsumer execinfra.RowReceiver,
 	batchSyncFlowConsumer execinfra.BatchReceiver,
@@ -501,7 +502,7 @@ func newFlow(
 	onFlowCleanup func(),
 	statementSQL string,
 ) flowinfra.Flow {
-	base := flowinfra.NewFlowBase(flowCtx, flowReg, rowSyncFlowConsumer, batchSyncFlowConsumer, localProcessors, onFlowCleanup, statementSQL)
+	base := flowinfra.NewFlowBase(flowCtx, sp, flowReg, rowSyncFlowConsumer, batchSyncFlowConsumer, localProcessors, onFlowCleanup, statementSQL)
 	if isVectorized {
 		return colflow.NewVectorizedFlow(base)
 	}
@@ -669,7 +670,7 @@ func (ds *ServerImpl) flowStreamInt(
 	}
 	defer cleanup()
 	log.VEventf(ctx, 1, "connected inbound stream %s/%d", flowID.Short(), streamID)
-	return streamStrategy.Run(f.AnnotateCtx(ctx), stream, msg, f)
+	return streamStrategy.Run(f.AmbientContext.AnnotateCtx(ctx), stream, msg, f)
 }
 
 // FlowStream is part of the execinfrapb.DistSQLServer interface.

pkg/sql/distsql/vectorized_panic_propagation_test.go

@@ -44,6 +44,7 @@ func TestNonVectorizedPanicDoesntHangServer(t *testing.T) {
 	}
 	base := flowinfra.NewFlowBase(
 		flowCtx,
+		nil, /* sp */
 		nil, /* flowReg */
 		nil, /* rowSyncFlowConsumer */
 		nil, /* batchSyncFlowConsumer */

pkg/sql/execinfra/flow_context.go

@@ -30,7 +30,7 @@ import (
 // FlowCtx encompasses the configuration parameters needed for various flow
 // components.
 type FlowCtx struct {
-	log.AmbientContext
+	AmbientContext log.AmbientContext
 
 	Cfg *ServerConfig
 

pkg/sql/flowinfra/flow.go

@@ -195,6 +195,10 @@ type FlowBase struct {
 	ctxCancel context.CancelFunc
 	ctxDone   <-chan struct{}
 
+	// sp is the span that this Flow runs in. Can be nil if no span was created
+	// for the flow. Flow.Cleanup() finishes it.
+	sp *tracing.Span
+
 	// spec is the request that produced this flow. Only used for debugging.
 	spec *execinfrapb.FlowSpec
 
@@ -246,8 +250,12 @@ func (f *FlowBase) Started() bool {
 var _ Flow = &FlowBase{}
 
 // NewFlowBase creates a new FlowBase.
+//
+// sp, if not nil, is the Span corresponding to the flow. The flow takes
+// ownership; Cleanup() will finish it.
 func NewFlowBase(
 	flowCtx execinfra.FlowCtx,
+	sp *tracing.Span,
 	flowReg *FlowRegistry,
 	rowSyncFlowConsumer execinfra.RowReceiver,
 	batchSyncFlowConsumer execinfra.BatchReceiver,
@@ -270,6 +278,7 @@ func NewFlowBase(
 	}
 	return &FlowBase{
 		FlowCtx:               flowCtx,
+		sp:                    sp,
 		flowRegistry:          flowReg,
 		rowSyncFlowConsumer:   rowSyncFlowConsumer,
 		batchSyncFlowConsumer: batchSyncFlowConsumer,
@@ -497,15 +506,14 @@ func (f *FlowBase) Cleanup(ctx context.Context) {
 		f.Descriptors.ReleaseAll(ctx)
 	}
 
-	sp := tracing.SpanFromContext(ctx)
-	if sp != nil {
-		defer sp.Finish()
+	if f.sp != nil {
+		defer f.sp.Finish()
 		if f.Gateway && f.CollectStats {
 			// If this is the gateway node and we're collecting execution stats,
 			// output the maximum memory usage to the flow span. Note that
 			// non-gateway nodes use the last outbox to send this information
 			// over.
-			sp.RecordStructured(&execinfrapb.ComponentStats{
+			f.sp.RecordStructured(&execinfrapb.ComponentStats{
 				Component: execinfrapb.FlowComponentID(f.NodeID.SQLInstanceID(), f.FlowCtx.ID),
 				FlowStats: execinfrapb.FlowStats{
 					MaxMemUsage:  optional.MakeUint(uint64(f.FlowCtx.EvalCtx.Mon.MaximumBytes())),