From 3f162f9ab6ba713cb5bcba28478ffdd9a7028392 Mon Sep 17 00:00:00 2001 From: Marylia Gutierrez 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 a033546acc99..a17eb1abbe4c 100644 --- a/docs/generated/settings/settings-for-tenants.txt +++ b/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 diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index d1d968623f48..678fac170bb7 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -166,7 +166,7 @@ sql.metrics.max_mem_txn_fingerprintsinteger100000the maximum number of transaction fingerprints stored in memory sql.metrics.statement_details.dump_to_logsbooleanfalsedump collected statement statistics to node logs when periodically cleared sql.metrics.statement_details.enabledbooleantruecollect per-statement query statistics -sql.metrics.statement_details.plan_collection.enabledbooleantrueperiodically save a logical plan for each fingerprint +sql.metrics.statement_details.plan_collection.enabledbooleanfalseperiodically save a logical plan for each fingerprint sql.metrics.statement_details.plan_collection.periodduration5m0sthe time until a new logical plan is collected sql.metrics.statement_details.thresholdduration0sminimum execution time to cause statement statistics to be collected. If configured, no transaction stats are collected. sql.metrics.transaction_details.enabledbooleantruecollect per-application transaction statistics 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 8edc3ea6f19c..e6c2eed07359 100644 --- a/pkg/server/status_test.go +++ b/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) } diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go index 901f28a03435..64a60cde88ea 100644 --- a/pkg/sql/instrumentation.go +++ b/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 } diff --git a/pkg/sql/sqlstats/cluster_settings.go b/pkg/sql/sqlstats/cluster_settings.go index c254a52e02ae..e87ac5fe20bf 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 ed8d83fa8007..5618e5631b11 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 26441ca9130e..361a486ba3e5 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)