From 373024c1e6b29f41ffaaac1cee143adc480bc598 Mon Sep 17 00:00:00 2001 From: j82w Date: Wed, 30 Aug 2023 23:11:51 +0000 Subject: [PATCH] sql: fix performance when disabling sql.metrics.statement_details.enabled There is a performance regression when disabling sql.metrics.statement_details.enabled. It will see that every execution is the first time the query was executed so it will enabled execution stats collection for all executions. This is a bug caused by always collecting the execution stats of the first time a statement is executed in #89418. Fixes: #106811 Release note (bug fix): Fixes a bug causing performance regression when disabling `sql.metrics.statement_details.enabled` which caused execution stats to be collected for all queries instead of the default one percent. --- pkg/sql/instrumentation.go | 4 +- .../sqlstats/persistedsqlstats/BUILD.bazel | 1 + .../persistedsqlstats/controller_test.go | 40 +++++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go index 9f71c90ac360..c3661254da02 100644 --- a/pkg/sql/instrumentation.go +++ b/pkg/sql/instrumentation.go @@ -448,7 +448,9 @@ func (ih *instrumentationHelper) Setup( ih.collectExecStats = collectTxnExecStats - if !collectTxnExecStats && !previouslySampled { + // Don't collect it if Stats Collection is disabled. If it is disabled the + // stats are not stored, so it always returns false for previouslySampled. + if !collectTxnExecStats && (!previouslySampled && sqlstats.StmtStatsEnable.Get(&cfg.Settings.SV)) { // We don't collect the execution stats for statements in this txn, but // this is the first time we see this statement ever, so we'll collect // its execution stats anyway (unless the user disabled txn stats diff --git a/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel b/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel index 68ce48da1869..774f4b415bf7 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel +++ b/pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel @@ -85,6 +85,7 @@ go_test( "//pkg/security/securitytest", "//pkg/security/username", "//pkg/server", + "//pkg/settings/cluster", "//pkg/sql", "//pkg/sql/appstatspb", "//pkg/sql/catalog", diff --git a/pkg/sql/sqlstats/persistedsqlstats/controller_test.go b/pkg/sql/sqlstats/persistedsqlstats/controller_test.go index 85eb6753219f..4eac861f2901 100644 --- a/pkg/sql/sqlstats/persistedsqlstats/controller_test.go +++ b/pkg/sql/sqlstats/persistedsqlstats/controller_test.go @@ -18,12 +18,14 @@ import ( "testing" "github.com/cockroachdb/cockroach/pkg/base" + "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats" "github.com/cockroachdb/cockroach/pkg/sql/tests" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" + "github.com/cockroachdb/cockroach/pkg/testutils/testcluster" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" @@ -260,3 +262,41 @@ func TestActivityTablesReset(t *testing.T) { sqlDB.QueryRow(t, "SELECT count(*) FROM system.transaction_activity").Scan(&count) require.Equal(t, 0 /* expected */, count) } + +func TestStmtStatsEnable(t *testing.T) { + defer leaktest.AfterTest(t)() + defer log.Scope(t).Close(t) + + // Start the cluster. (One node is sufficient; the outliers system is currently in-memory only.) + ctx := context.Background() + settings := cluster.MakeTestingClusterSettings() + args := base.TestClusterArgs{ServerArgs: base.TestServerArgs{Settings: settings}} + tc := testcluster.StartTestCluster(t, 1, args) + defer tc.Stopper().Stop(ctx) + + serverutils.SetClusterSetting(t, tc, "sql.metrics.statement_details.enabled", "false") + + sqlConn := sqlutils.MakeSQLRunner(tc.ServerConn(0)) + + sqlConn.Exec(t, "SELECT crdb_internal.reset_sql_stats()") + + appName := "TestStmtStatsEnable" + sqlConn.Exec(t, "SET application_name = $1", appName) + + sqlConn.Exec(t, "SELECT count_rows() FROM crdb_internal.statement_statistics") + sqlConn.Exec(t, "SELECT count_rows() FROM crdb_internal.transaction_statistics") + sqlConn.Exec(t, "SELECT count_rows() FROM crdb_internal.statement_statistics WHERE app_name = $1", appName) + sqlConn.Exec(t, "SELECT count_rows() FROM crdb_internal.transaction_statistics WHERE app_name = $1", appName) + + sqlConn.Exec(t, "SET application_name = $1", "ObserverTestStmtStatsEnable") + var count int + sqlConn.QueryRow(t, + "SELECT count(*) FROM crdb_internal.statement_statistics WHERE app_name = $1", appName). + Scan(&count) + require.Equal(t, 0 /* expected */, count) + + sqlConn.QueryRow(t, + "SELECT count(*) FROM crdb_internal.transaction_statistics WHERE app_name = $1 AND (statistics->'execution_statistics'->>'cnt')::int > 0 ", appName). + Scan(&count) + require.Less(t, count, 4, "statement execution stats collection is disabled there should be less than 4 rows. Actual: %d", count) +}