From c1adb2d5eaef13a56fe10578ba7be873c4f29745 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. --- docs/generated/settings/settings-for-tenants.txt | 2 +- docs/generated/settings/settings.html | 2 +- pkg/ccl/telemetryccl/telemetry_logging_test.go | 1 + pkg/server/status_test.go | 9 --------- pkg/sql/sqlstats/cluster_settings.go | 2 +- pkg/sql/sqlstats/persistedsqlstats/datadriven_test.go | 4 ++++ pkg/util/log/logcrash/crash_reporting_packet_test.go | 4 ++++ 7 files changed, 12 insertions(+), 12 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 @@ sql.metrics.statement_details.enabledbooleantruecollect per-statement query statistics sql.metrics.statement_details.index_recommendation_collection.enabledbooleantruegenerate an index recommendation for each fingerprint ID sql.metrics.statement_details.max_mem_reported_idx_recommendationsinteger5000the maximum number of reported index recommendation info stored in memory -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..6b9db11e4ccb 100644 --- a/pkg/ccl/telemetryccl/telemetry_logging_test.go +++ b/pkg/ccl/telemetryccl/telemetry_logging_test.go @@ -56,6 +56,7 @@ func TestTelemetryLogRegions(t *testing.T) { // 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;`) + sqlDB.Exec(t, `SET CLUSTER SETTING sql.metrics.statement_details.plan_collection.enabled = true;`) sqlDB.Exec(t, `SET CLUSTER SETTING sql.telemetry.query_sampling.max_event_frequency = 1000000`) testData := []struct { 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/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)