Merge cockroachdb#35425

35425: stats: improve UI experience r=RaduBerinde a=RaduBerinde

This change makes the following improvements:

 - the job name is now a user-friendly message (instead of the exact SQL
   statement) for automatic table stats. The message includes the
   fully-qualified tale name (the table name was not visible at all).

 - the event description in the UI is fixed (used to be an "unknown
   event error"); the message includes the FQ table name.

 - events are not posted by default (they are too spammy and redundant
   with the jobs); but they can be turned on via a cluster setting
   (could be useful during debugging).

Release note (admin ui change): CREATE STATISTICS jobs no longer
generate events by default; improved descriptions for automatic
statistics jobs.

Fixes cockroachdb#35319.

Co-authored-by: Radu Berinde <[email protected]>

docs/generated/settings/settings.html

@@ -88,6 +88,7 @@
 <tr><td><code>sql.query_cache.enabled</code></td><td>boolean</td><td><code>true</code></td><td>enable the query cache</td></tr>
 <tr><td><code>sql.stats.experimental_automatic_collection.enabled</code></td><td>boolean</td><td><code>true</code></td><td>experimental automatic statistics collection mode</td></tr>
 <tr><td><code>sql.stats.experimental_automatic_collection.fraction_idle</code></td><td>float</td><td><code>0.9</code></td><td>fraction of time that automatic statistics sampler processors are idle</td></tr>
+<tr><td><code>sql.stats.post_events</code></td><td>boolean</td><td><code>false</code></td><td>if set, an event is shown for every CREATE STATISTICS job</td></tr>
 <tr><td><code>sql.tablecache.lease.refresh_limit</code></td><td>integer</td><td><code>50</code></td><td>maximum number of tables to periodically refresh leases for</td></tr>
 <tr><td><code>sql.trace.log_statement_execute</code></td><td>boolean</td><td><code>false</code></td><td>set to true to enable logging of executed statements</td></tr>
 <tr><td><code>sql.trace.session_eventlog.enabled</code></td><td>boolean</td><td><code>false</code></td><td>set to true to enable session tracing</td></tr>

pkg/jobs/jobspb/jobs.proto

@@ -209,13 +209,14 @@ message CreateStatsDetails {
       (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sqlbase.ColumnID"
     ];
   }
-  string name = 1 [
-    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/sem/tree.Name"
-  ];
+  string name = 1;
   sqlbase.TableDescriptor table = 2 [(gogoproto.nullable) = false];
   repeated ColList column_lists = 3 [(gogoproto.nullable) = false];
   string statement = 4;
   util.hlc.Timestamp as_of = 5;
+
+  // Fully qualified table name.
+  string fq_table_name = 6 [(gogoproto.customname) = "FQTableName"];
 }
 
 message CreateStatsProgress {

pkg/sql/create_stats.go

@@ -16,10 +16,12 @@ package sql
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/internal/client"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
@@ -34,6 +36,14 @@ import (
 	"google.golang.org/grpc/status"
 )
 
+// createStatsPostEvents controls the cluster setting for enabling
+// automatic table statistics collection.
+var createStatsPostEvents = settings.RegisterBoolSetting(
+	"sql.stats.post_events",
+	"if set, an event is shown for every CREATE STATISTICS job",
+	false,
+)
+
 func (p *planner) CreateStatistics(ctx context.Context, n *tree.CreateStats) (planNode, error) {
 	return &createStatsNode{
 		CreateStats: *n,
@@ -97,6 +107,7 @@ func (n *createStatsNode) startJob(ctx context.Context, resultsCh chan<- tree.Da
 	}
 
 	var tableDesc *ImmutableTableDescriptor
+	var fqTableName string
 	var err error
 	switch t := n.Table.(type) {
 	case *tree.TableName:
@@ -107,6 +118,7 @@ func (n *createStatsNode) startJob(ctx context.Context, resultsCh chan<- tree.Da
 		if err != nil {
 			return err
 		}
+		fqTableName = t.FQString()
 
 	case *tree.TableRef:
 		flags := ObjectLookupFlags{CommonLookupFlags: CommonLookupFlags{
@@ -116,6 +128,10 @@ func (n *createStatsNode) startJob(ctx context.Context, resultsCh chan<- tree.Da
 		if err != nil {
 			return err
 		}
+		fqTableName, err = n.p.getQualifiedTableName(ctx, &tableDesc.TableDescriptor)
+		if err != nil {
+			return err
+		}
 	}
 
 	if tableDesc.IsVirtualTable() {
@@ -173,11 +189,21 @@ func (n *createStatsNode) startJob(ctx context.Context, resultsCh chan<- tree.Da
 	}
 
 	// Create a job to run statistics creation.
+	var description string
+	if n.Name == stats.AutoStatsName {
+		// Use a user-friendly description for automatic statistics.
+		description = fmt.Sprintf("Table statistics refresh for %s", fqTableName)
+	} else {
+		// This must be a user query, so use the statement (for consistency with
+		// other jobs triggered by statements).
+		description = tree.AsStringWithFlags(n, tree.FmtAlwaysQualifyTableNames)
+	}
 	_, errCh, err := n.p.ExecCfg().JobRegistry.StartJob(ctx, resultsCh, jobs.Record{
-		Description: tree.AsStringWithFlags(n, tree.FmtAlwaysQualifyTableNames),
+		Description: description,
 		Username:    n.p.User(),
 		Details: jobspb.CreateStatsDetails{
-			Name:        n.Name,
+			Name:        string(n.Name),
+			FQTableName: fqTableName,
 			Table:       tableDesc.TableDescriptor,
 			ColumnLists: createStatsColLists,
 			Statement:   n.String(),
@@ -380,6 +406,10 @@ func (r *createStatsResumer) OnFailOrCancel(
 // OnSuccess is part of the jobs.Resumer interface.
 func (r *createStatsResumer) OnSuccess(ctx context.Context, _ *client.Txn, job *jobs.Job) error {
 	details := job.Details().(jobspb.CreateStatsDetails)
+
+	if !createStatsPostEvents.Get(&r.evalCtx.Settings.SV) {
+		return nil
+	}
 	// Record this statistics creation in the event log.
 	// TODO(rytaft): This creates a new transaction for the CREATE STATISTICS
 	// event. It must be different from the CREATE STATISTICS transaction,
@@ -395,9 +425,9 @@ func (r *createStatsResumer) OnSuccess(ctx context.Context, _ *client.Txn, job *
 			int32(details.Table.ID),
 			int32(r.evalCtx.NodeID),
 			struct {
-				StatisticName string
-				Statement     string
-			}{details.Name.String(), details.Statement},
+				TableName string
+				Statement string
+			}{details.FQTableName, details.Statement},
 		)
 	})
 }

pkg/sql/distsql_plan_stats.go

@@ -171,7 +171,7 @@ func (dsp *DistSQLPlanner) createPlanForCreateStats(
 			columns:             details.ColumnLists[i].IDs,
 			histogram:           histogram,
 			histogramMaxBuckets: histogramBuckets,
-			name:                string(details.Name),
+			name:                details.Name,
 		}
 	}
 

pkg/sql/logictest/testdata/logic_test/distsql_event_log

@@ -4,6 +4,10 @@
 # CREATE STATISTICS
 ###################
 
+# This test verifies that events are posted for table statistics creation.
+statement ok
+SET CLUSTER SETTING sql.stats.post_events = TRUE
+
 statement ok
 CREATE TABLE a (id INT PRIMARY KEY, x INT, y INT, INDEX x_idx (x, y))
 
@@ -13,16 +17,14 @@ CREATE STATISTICS s1 ON id FROM a
 statement ok
 CREATE STATISTICS __auto__ FROM a
 
-# verify statistics creation is logged
-##################
 query IIT
 SELECT "targetID", "reportingID", "info"
 FROM system.eventlog
 WHERE "eventType" = 'create_statistics'
 ORDER BY "timestamp"
 ----
-53  1  {"StatisticName":"s1","Statement":"CREATE STATISTICS s1 ON id FROM a"}
-53  1  {"StatisticName":"__auto__","Statement":"CREATE STATISTICS __auto__ FROM a"}
+53  1  {"TableName":"test.public.a","Statement":"CREATE STATISTICS s1 ON id FROM a"}
+53  1  {"TableName":"test.public.a","Statement":"CREATE STATISTICS __auto__ FROM a"}
 
 statement ok
 DROP TABLE a

pkg/ui/src/util/eventTypes.ts

@@ -69,6 +69,8 @@ export const SET_CLUSTER_SETTING = "set_cluster_setting";
 export const SET_ZONE_CONFIG = "set_zone_config";
 // Recorded when a zone config is removed.
 export const REMOVE_ZONE_CONFIG = "remove_zone_config";
+// Recorded when statistics are collected for a table.
+export const CREATE_STATISTICS = "create_statistics";
 
 // Node Event Types
 export const nodeEvents = [NODE_JOIN, NODE_RESTART, NODE_DECOMMISSIONED, NODE_RECOMMISSIONED];

pkg/ui/src/util/events.ts

@@ -76,9 +76,11 @@ export function getEventDescription(e: Event$Properties): string {
       }
       return `Cluster Setting Changed: User ${info.User} changed ${info.SettingName}`;
     case eventTypes.SET_ZONE_CONFIG:
-    return `Zone Config Changed: User ${info.User} set the zone config for ${info.Target} to ${info.Config}`;
+      return `Zone Config Changed: User ${info.User} set the zone config for ${info.Target} to ${info.Config}`;
     case eventTypes.REMOVE_ZONE_CONFIG:
       return `Zone Config Removed: User ${info.User} removed the zone config for ${info.Target}`;
+    case eventTypes.CREATE_STATISTICS:
+      return `Table statistics refreshed for ${info.TableName}`;
     default:
       return `Unknown Event Type: ${e.event_type}, content: ${JSON.stringify(info, null, 2)}`;
   }
@@ -98,6 +100,7 @@ export interface EventInfo {
   Value?: string;
   Target?: string;
   Config?: string;
+  Statement?: string;
   // The following are three names for the same key (it was renamed twice).
   // All ar included for backwards compatibility.
   DroppedTables?: string[];