From 0a0895fb220d45a3bda54ff28ea85e47c124a24a Mon Sep 17 00:00:00 2001
From: Marylia Gutierrez <marylia@cockroachlabs.com>
Date: Wed, 21 Sep 2022 09:36:33 -0400
Subject: [PATCH] sql: turn off plan sampling by default

Previously, we were sampling plans for fingerprints
and saving to statement_statistics tables. Now that
we are saving plan hash and plan gist (that allow us
to decode back to the plan) we are no longer using the
sampled plan anywhere. Since this is a heavy opperation,
we are turning it off by default, but changing the default
value of `sql.metrics.statement_details.plan_collection.enabled`
to `false`.
If we don't receive feedback about turning it back on,
we can remove this sampling entirely.

Partially addresses #77944

Release note (sql change): Change the default value of
`sql.metrics.statement_details.plan_collection.enabled`
to false, since we no longer use this information
anywhere.
---
 .../settings/settings-for-tenants.txt          |  2 +-
 docs/generated/settings/settings.html          |  2 +-
 pkg/ccl/telemetryccl/telemetry_logging_test.go |  4 ++++
 pkg/server/status_test.go                      |  9 ---------
 pkg/sql/instrumentation.go                     | 18 +++++++++---------
 pkg/sql/sqlstats/cluster_settings.go           |  2 +-
 .../persistedsqlstats/datadriven_test.go       |  4 ++++
 .../logcrash/crash_reporting_packet_test.go    |  4 ++++
 8 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/docs/generated/settings/settings-for-tenants.txt b/docs/generated/settings/settings-for-tenants.txt
index 1cdd10b82c2a..a12376609ac7 100644
--- a/docs/generated/settings/settings-for-tenants.txt
+++ b/docs/generated/settings/settings-for-tenants.txt
@@ -248,7 +248,7 @@ sql.metrics.statement_details.dump_to_logs	boolean	false	dump collected statemen
 sql.metrics.statement_details.enabled	boolean	true	collect per-statement query statistics
 sql.metrics.statement_details.index_recommendation_collection.enabled	boolean	true	generate an index recommendation for each fingerprint ID
 sql.metrics.statement_details.max_mem_reported_idx_recommendations	integer	5000	the maximum number of reported index recommendation info stored in memory
-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
diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html
index 71ed48a29a03..3fba16ed6592 100644
--- a/docs/generated/settings/settings.html
+++ b/docs/generated/settings/settings.html
@@ -182,7 +182,7 @@
 <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.index_recommendation_collection.enabled</code></td><td>boolean</td><td><code>true</code></td><td>generate an index recommendation for each fingerprint ID</td></tr>
 <tr><td><code>sql.metrics.statement_details.max_mem_reported_idx_recommendations</code></td><td>integer</td><td><code>5000</code></td><td>the maximum number of reported index recommendation info stored in memory</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>
diff --git a/pkg/ccl/telemetryccl/telemetry_logging_test.go b/pkg/ccl/telemetryccl/telemetry_logging_test.go
index 9bc549d6292c..127a1fe47c69 100644
--- a/pkg/ccl/telemetryccl/telemetry_logging_test.go
+++ b/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;`)
diff --git a/pkg/server/status_test.go b/pkg/server/status_test.go
index a3ab1fb5c88a..0cdc11f1e7fd 100644
--- a/pkg/server/status_test.go
+++ b/pkg/server/status_test.go
@@ -1818,10 +1818,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)
 		}
 
@@ -1934,11 +1930,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)
 		}
 
diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go
index 8224ea387cbb..f2a0ae0b8793 100644
--- a/pkg/sql/instrumentation.go
+++ b/pkg/sql/instrumentation.go
@@ -257,11 +257,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() {
@@ -307,9 +309,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(tracingpb.RecordingVerbose))
 	ih.shouldFinishSpan = true
@@ -439,7 +438,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
 }
 
diff --git a/pkg/sql/sqlstats/cluster_settings.go b/pkg/sql/sqlstats/cluster_settings.go
index 74e9d2bb6be3..693df3bc23bd 100644
--- a/pkg/sql/sqlstats/cluster_settings.go
+++ b/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
diff --git a/pkg/sql/sqlstats/persistedsqlstats/datadriven_test.go b/pkg/sql/sqlstats/persistedsqlstats/datadriven_test.go
index 88e8085100b6..d418b1de14cf 100644
--- a/pkg/sql/sqlstats/persistedsqlstats/datadriven_test.go
+++ b/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 {
diff --git a/pkg/util/log/logcrash/crash_reporting_packet_test.go b/pkg/util/log/logcrash/crash_reporting_packet_test.go
index e4ccc9c109f9..48889458a7cd 100644
--- a/pkg/util/log/logcrash/crash_reporting_packet_test.go
+++ b/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)