Merge pull request #88420 from maryliag/backport22.1-88343

release-22.1: sql: turn off plan sampling by default

docs/generated/settings/settings-for-tenants.txt

@@ -150,7 +150,7 @@ sql.metrics.max_mem_stmt_fingerprints	integer	100000	the maximum number of state
 sql.metrics.max_mem_txn_fingerprints	integer	100000	the maximum number of transaction fingerprints stored in memory
 sql.metrics.statement_details.dump_to_logs	boolean	false	dump collected statement statistics to node logs when periodically cleared
 sql.metrics.statement_details.enabled	boolean	true	collect per-statement query statistics
-sql.metrics.statement_details.plan_collection.enabled	boolean	true	periodically save a logical plan for each fingerprint
+sql.metrics.statement_details.plan_collection.enabled	boolean	false	periodically save a logical plan for each fingerprint
 sql.metrics.statement_details.plan_collection.period	duration	5m0s	the time until a new logical plan is collected
 sql.metrics.statement_details.threshold	duration	0s	minimum execution time to cause statement statistics to be collected. If configured, no transaction stats are collected.
 sql.metrics.transaction_details.enabled	boolean	true	collect per-application transaction statistics

docs/generated/settings/settings.html

@@ -166,7 +166,7 @@
 <tr><td><code>sql.metrics.max_mem_txn_fingerprints</code></td><td>integer</td><td><code>100000</code></td><td>the maximum number of transaction fingerprints stored in memory</td></tr>
 <tr><td><code>sql.metrics.statement_details.dump_to_logs</code></td><td>boolean</td><td><code>false</code></td><td>dump collected statement statistics to node logs when periodically cleared</td></tr>
 <tr><td><code>sql.metrics.statement_details.enabled</code></td><td>boolean</td><td><code>true</code></td><td>collect per-statement query statistics</td></tr>
-<tr><td><code>sql.metrics.statement_details.plan_collection.enabled</code></td><td>boolean</td><td><code>true</code></td><td>periodically save a logical plan for each fingerprint</td></tr>
+<tr><td><code>sql.metrics.statement_details.plan_collection.enabled</code></td><td>boolean</td><td><code>false</code></td><td>periodically save a logical plan for each fingerprint</td></tr>
 <tr><td><code>sql.metrics.statement_details.plan_collection.period</code></td><td>duration</td><td><code>5m0s</code></td><td>the time until a new logical plan is collected</td></tr>
 <tr><td><code>sql.metrics.statement_details.threshold</code></td><td>duration</td><td><code>0s</code></td><td>minimum execution time to cause statement statistics to be collected. If configured, no transaction stats are collected.</td></tr>
 <tr><td><code>sql.metrics.transaction_details.enabled</code></td><td>boolean</td><td><code>true</code></td><td>collect per-application transaction statistics</td></tr>

pkg/ccl/telemetryccl/telemetry_logging_test.go

@@ -53,6 +53,10 @@ func TestTelemetryLogRegions(t *testing.T) {
 	sqlDB.Exec(t, `ALTER TABLE three_regions SPLIT AT SELECT generate_series(1, 3)`)
 	sqlDB.Exec(t, "ALTER TABLE three_regions EXPERIMENTAL_RELOCATE VALUES (ARRAY[1], 1), (ARRAY[2], 2), (ARRAY[3], 3)")
 
+	// Enable the sampling of all statements so that execution statistics
+	// (including the regions information) is collected.
+	sqlDB.Exec(t, `SET CLUSTER SETTING sql.txn_stats.sample_rate = 1.0`)
+
 	// Enable the telemetry logging and increase the sampling frequency so that
 	// all statements are captured.
 	sqlDB.Exec(t, `SET CLUSTER SETTING sql.telemetry.query_sampling.enabled = true;`)

pkg/server/status_test.go

@@ -1945,10 +1945,6 @@ func TestStatusAPIStatements(t *testing.T) {
 				// Ignore the ALTER USER ... VIEWACTIVITY statement.
 				continue
 			}
-			if len(respStatement.Stats.SensitiveInfo.MostRecentPlanDescription.Name) == 0 {
-				// Ensure that we populate the explain plan.
-				t.Fatal("expected MostRecentPlanDescription to be populated")
-			}
 			statementsInResponse = append(statementsInResponse, respStatement.Key.KeyData.Query)
 		}
 
@@ -2061,11 +2057,6 @@ func TestStatusAPICombinedStatements(t *testing.T) {
 				continue
 			}
 
-			if len(respStatement.Stats.SensitiveInfo.MostRecentPlanDescription.Name) == 0 {
-				// Ensure that we populate the explain plan.
-				t.Fatal("expected MostRecentPlanDescription to be populated")
-			}
-
 			statementsInResponse = append(statementsInResponse, respStatement.Key.KeyData.Query)
 		}
 

pkg/sql/instrumentation.go

@@ -220,11 +220,13 @@ func (ih *instrumentationHelper) Setup(
 	ih.savePlanForStats =
 		statsCollector.ShouldSaveLogicalPlanDesc(fingerprint, implicitTxn, p.SessionData().Database)
 
-	if ih.ShouldBuildExplainPlan() {
-		// Populate traceMetadata early in case we short-circuit the execution
-		// before reaching the bottom of this method.
-		ih.traceMetadata = make(execNodeTraceMetadata)
-	}
+	defer func() {
+		if ih.ShouldBuildExplainPlan() {
+			// Populate traceMetadata at the end once we have all properties of
+			// the helper setup.
+			ih.traceMetadata = make(execNodeTraceMetadata)
+		}
+	}()
 
 	if sp := tracing.SpanFromContext(ctx); sp != nil {
 		if sp.IsVerbose() {
@@ -270,9 +272,6 @@ func (ih *instrumentationHelper) Setup(
 	}
 
 	ih.collectExecStats = true
-	if ih.traceMetadata == nil {
-		ih.traceMetadata = make(execNodeTraceMetadata)
-	}
 	ih.evalCtx = p.EvalContext()
 	newCtx, ih.sp = tracing.EnsureChildSpan(ctx, cfg.AmbientCtx.Tracer, "traced statement", tracing.WithRecording(tracing.RecordingVerbose))
 	ih.shouldFinishSpan = true
@@ -424,7 +423,8 @@ func (ih *instrumentationHelper) ShouldUseJobForCreateStats() bool {
 // ShouldBuildExplainPlan returns true if we should build an explain plan and
 // call RecordExplainPlan.
 func (ih *instrumentationHelper) ShouldBuildExplainPlan() bool {
-	return ih.collectBundle || ih.savePlanForStats || ih.outputMode == explainAnalyzePlanOutput ||
+	return ih.collectBundle || ih.collectExecStats || ih.savePlanForStats ||
+		ih.outputMode == explainAnalyzePlanOutput ||
 		ih.outputMode == explainAnalyzeDistSQLOutput
 }
 

pkg/sql/sqlstats/cluster_settings.go

@@ -65,7 +65,7 @@ var SampleLogicalPlans = settings.RegisterBoolSetting(
 	settings.TenantWritable,
 	"sql.metrics.statement_details.plan_collection.enabled",
 	"periodically save a logical plan for each fingerprint",
-	true,
+	false,
 ).WithPublic()
 
 // LogicalPlanCollectionPeriod specifies the interval between collections of

pkg/sql/sqlstats/persistedsqlstats/datadriven_test.go

@@ -93,6 +93,10 @@ func TestSQLStatsDataDriven(t *testing.T) {
 	observerConn := cluster.ServerConn(1 /* idx */)
 
 	observer := sqlutils.MakeSQLRunner(observerConn)
+	_, err := sqlConn.Exec(`SET CLUSTER SETTING sql.metrics.statement_details.plan_collection.enabled = true;`)
+	if err != nil {
+		t.Errorf("failed to enable plan collection due to %s", err.Error())
+	}
 
 	execDataDrivenTestCmd := func(t *testing.T, d *datadriven.TestData) string {
 		switch d.Cmd {

pkg/util/log/logcrash/crash_reporting_packet_test.go

@@ -214,6 +214,10 @@ func TestInternalErrorReporting(t *testing.T) {
 
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(ctx)
+	_, err := sqlDB.Exec(`SET CLUSTER SETTING sql.metrics.statement_details.plan_collection.enabled = true;`)
+	if err != nil {
+		t.Errorf("failed to enable plan collection due to %s", err.Error())
+	}
 
 	if _, err := sqlDB.Exec("SELECT crdb_internal.force_assertion_error('woo')"); !testutils.IsError(err, "internal error") {
 		t.Fatalf("expected internal error, got %v", err)