sql: fix incorrect follower read and testing knob for sql stats

compaction job

Previously, the SQL Stats Compaction job did not check for nil
testing knobs and constructed its SQL query with
incorrect follower read syntax
This commit address those two bugs.

Release justification: Category 2: Bug fixes and low-risk updates
to new functionality

Release note: None

pkg/sql/sqlstats/persistedsqlstats/BUILD.bazel

@@ -64,6 +64,7 @@ go_test(
  "//pkg/security/securitytest",
  "//pkg/server",
  "//pkg/sql",
+ "//pkg/sql/catalog",
  "//pkg/sql/catalog/descpb",
  "//pkg/sql/sessiondata",
  "//pkg/sql/sqlstats",

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go

@@ -191,20 +191,21 @@ func (c *StatsCompactor) getQueryForCheckingTableRowCounts(
  tableName, hashColumnName string,
 ) string {
  // [1]: table name
- // [2]: hash column name
- // [3]: follower read clause
+ // [2]: follower read clause
+ // [3]: hash column name
  existingRowCountQuery := `
 SELECT count(*)
 FROM %[1]s
-WHERE %[2]s = $1
-%[3]s`
+%[2]s
+WHERE %[3]s = $1
+`
  followerReadClause := "AS OF SYSTEM TIME follower_read_timestamp()"
 
- if c.knobs.DisableFollowerRead {
- followerReadClause = ""
+ if c.knobs != nil {
+ followerReadClause = c.knobs.AOSTClause
  }
 
- return fmt.Sprintf(existingRowCountQuery, tableName, hashColumnName, followerReadClause)
+ return fmt.Sprintf(existingRowCountQuery, tableName, followerReadClause, hashColumnName)
 }
 
 func (c *StatsCompactor) getStatementForDeletingStaleRows(

pkg/sql/sqlstats/persistedsqlstats/compaction_test.go

@@ -25,6 +25,7 @@ import (
  "github.com/cockroachdb/cockroach/pkg/roachpb"
  "github.com/cockroachdb/cockroach/pkg/security"
  "github.com/cockroachdb/cockroach/pkg/sql"
+ "github.com/cockroachdb/cockroach/pkg/sql/catalog"
  "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
  "github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
  "github.com/cockroachdb/cockroach/pkg/sql/sqlstats/persistedsqlstats"
@@ -39,6 +40,30 @@ import (
  "github.com/stretchr/testify/require"
 )
 
+func TestSQLStatsCompactorNilTestingKnobCheck(t *testing.T) {
+ defer leaktest.AfterTest(t)()
+ defer log.Scope(t).Close(t)
+
+ ctx := context.Background()
+ server, _, _ := serverutils.StartServer(t, base.TestServerArgs{})
+ defer server.Stopper().Stop(ctx)
+
+ statsCompactor := persistedsqlstats.NewStatsCompactor(
+ server.ClusterSettings(),
+ server.InternalExecutor().(sqlutil.InternalExecutor),
+ server.DB(),
+ metric.NewCounter(metric.Metadata{}),
+ nil, /* knobs */
+ )
+
+ // We run the compactor without disabling the follower read. This can possibly
+ // fail due to descriptor not found.
+ err := statsCompactor.DeleteOldestEntries(ctx)
+ if err != nil {
+ require.ErrorIs(t, err, catalog.ErrDescriptorNotFound)
+ }
+}
+
 func TestSQLStatsCompactor(t *testing.T) {
  defer leaktest.AfterTest(t)()
  defer log.Scope(t).Close(t)
@@ -64,7 +89,7 @@ func TestSQLStatsCompactor(t *testing.T) {
  ServerArgs: base.TestServerArgs{
  Knobs: base.TestingKnobs{
  SQLStatsKnobs: &persistedsqlstats.TestingKnobs{
- DisableFollowerRead: true,
+ AOSTClause: "AS OF SYSTEM TIME '-1us'",
  },
  },
  },
@@ -135,7 +160,7 @@ func TestSQLStatsCompactor(t *testing.T) {
  firstServer.DB(),
  metric.NewCounter(metric.Metadata{}),
  &persistedsqlstats.TestingKnobs{
- DisableFollowerRead: true,
+ AOSTClause: "AS OF SYSTEM TIME '-1us'",
  },
  )
 
@@ -183,7 +208,7 @@ func TestAtMostOneSQLStatsCompactionJob(t *testing.T) {
  var allowRequest chan struct{}
 
  serverArgs.Knobs.SQLStatsKnobs = &persistedsqlstats.TestingKnobs{
- DisableFollowerRead: true,
+ AOSTClause: "AS OF SYSTEM TIME '-1us'",
  }
 
  params := base.TestClusterArgs{ServerArgs: serverArgs}

pkg/sql/sqlstats/persistedsqlstats/reader_test.go

@@ -44,8 +44,8 @@ func TestPersistedSQLStatsRead(t *testing.T) {
  ServerArgs: base.TestServerArgs{
  Knobs: base.TestingKnobs{
  SQLStatsKnobs: &persistedsqlstats.TestingKnobs{
- StubTimeNow:  fakeTime.StubTimeNow,
- DisableFollowerRead: true,
+ StubTimeNow: fakeTime.StubTimeNow,
+ AOSTClause: "AS OF SYSTEM TIME '-1us'",
  },
  },
  },

pkg/sql/sqlstats/persistedsqlstats/stmt_reader.go

@@ -109,17 +109,24 @@ func (s *PersistedSQLStats) getFetchQueryForStmtStatsTable(
  "statistics",
  "plan",
  }
- query = fmt.Sprintf(`
+
+ // [1]: selection columns
+ // [2]: AOST clause
+ query = `
 SELECT 
- %s
+ %[1]s
 FROM
  system.statement_statistics
-`, strings.Join(selectedColumns, ","))
+%[2]s`
+
+ followerReadClause := "AS OF SYSTEM TIME follower_read_timestamp()"
 
- if s.cfg.Knobs != nil && !s.cfg.Knobs.DisableFollowerRead {
- query = fmt.Sprintf("%s AS OF SYSTEM TIME follower_read_timestamp()", query)
+ if s.cfg.Knobs != nil {
+ followerReadClause = s.cfg.Knobs.AOSTClause
  }
 
+ query = fmt.Sprintf(query, strings.Join(selectedColumns, ","), followerReadClause)
+
  orderByColumns := []string{"aggregated_ts"}
  if options.SortedAppNames {
  orderByColumns = append(orderByColumns, "app_name")

pkg/sql/sqlstats/persistedsqlstats/test_utils.go

@@ -22,11 +22,9 @@ type TestingKnobs struct {
  // by the flush operation to calculate aggregated_ts timestamp.
  StubTimeNow func() time.Time
 
- // DisableFollowerRead disallows the PersistedSQLStats to use follower read.
- // This is used in the unit tests where it might be reading from the past
- // where the stmt/txn stats system table are not yet created. This is not a
- // scenario that is possible in outside of testing.
- DisableFollowerRead bool
+ // AOSTClause overrides the AS OF SYSTEM TIME clause in queries used in
+ // persistedsqlstats.
+ AOSTClause string
 }
 
 // ModuleTestingKnobs implements base.ModuleTestingKnobs interface.

pkg/sql/sqlstats/persistedsqlstats/txn_reader.go

@@ -106,17 +106,24 @@ func (s *PersistedSQLStats) getFetchQueryForTxnStatsTable(
  "metadata",
  "statistics",
  }
- query = fmt.Sprintf(`
+
+ // [1]: selection columns
+ // [2]: AOST clause
+ query = `
 SELECT 
- %s
+ %[1]s
 FROM
  system.transaction_statistics
-`, strings.Join(selectedColumns, ","))
+%[2]s`
+
+ followerReadClause := "AS OF SYSTEM TIME follower_read_timestamp()"
 
- if s.cfg.Knobs != nil && !s.cfg.Knobs.DisableFollowerRead {
- query = fmt.Sprintf("%s AS OF SYSTEM TIME follower_read_timestamp()", query)
+ if s.cfg.Knobs != nil {
+ followerReadClause = s.cfg.Knobs.AOSTClause
  }
 
+ query = fmt.Sprintf(query, strings.Join(selectedColumns, ","), followerReadClause)
+
  orderByColumns := []string{"aggregated_ts"}
  if options.SortedAppNames {
  orderByColumns = append(orderByColumns, "app_name")