sql: fix performance when disabling sql.metrics.statement_details.ena…

…bled

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

@@ -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

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",

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)
+}