Skip to content

Commit

Permalink
sql: only record index reads on non-internal sessions
Browse files Browse the repository at this point in the history
Resolves: #77598

Previously, explicit `CREATE INDEX` queries were causing unintended
index reads in our `index_usage_statistics` subsystem. The unintended
index reads were caused by an index validation query from an internal
executor that did not use an internal planner. However, the internal
executor does set its session data as internal. This change adds a check
to only record index reads when the planner is not internal AND the
session is not internal.

Release note (bug fix): Fixed unintended recordings of index reads
caused by internal executor/queries.
  • Loading branch information
THardy98 committed Sep 28, 2022
1 parent c3a59b6 commit 3c6d430
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 7 deletions.
60 changes: 58 additions & 2 deletions pkg/server/index_usage_stats_test.go
Expand Up @@ -58,7 +58,7 @@ func TestStatusAPIIndexUsage(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)

testCluster := serverutils.StartNewTestCluster(t, 3, base.TestClusterArgs{})
testCluster := serverutils.StartNewTestCluster(t, 4, base.TestClusterArgs{})

ctx := context.Background()
defer testCluster.Stopper().Stop(ctx)
Expand Down Expand Up @@ -168,6 +168,55 @@ func TestStatusAPIIndexUsage(t *testing.T) {
_, err = secondServerSQLConn.Exec("EXPLAIN SELECT k, a FROM t WHERE a = 0")
require.NoError(t, err)

// Run some queries on the fourth node.
fourthServer := testCluster.Server(3 /* idx */)
fourthLocalStatsReader := fourthServer.SQLServer().(*sql.Server).GetLocalIndexStatistics()

fourthPgURL, fourthServerConnCleanup := sqlutils.PGUrl(
t, fourthServer.ServingSQLAddr(), "CreateConnections" /* prefix */, url.User(username.RootUser))
defer fourthServerConnCleanup()

fourthServerSQLConn, err := gosql.Open("postgres", fourthPgURL.String())
require.NoError(t, err)

defer func() {
err := fourthServerSQLConn.Close()
require.NoError(t, err)
}()

// Test that total_reads / last_read was not populated by an explicit CREATE INDEX query.
_, err = fourthServerSQLConn.Exec("CREATE TABLE test(num INT PRIMARY KEY, letter CHAR)")
require.NoError(t, err)

_, err = fourthServerSQLConn.Exec("CREATE INDEX ON test(letter)")
require.NoError(t, err)

// We fetch the table ID of the testing table.
fourthNodeRows, err := fourthServerSQLConn.Query("SELECT table_id FROM crdb_internal.tables WHERE name = 'test'")
require.NoError(t, err)
require.NotNil(t, fourthNodeRows)

defer func() {
err := fourthNodeRows.Close()
require.NoError(t, err)
}()

var testTableID int
require.True(t, fourthNodeRows.Next())
err = fourthNodeRows.Scan(&testTableID)
require.NoError(t, err)
require.False(t, fourthNodeRows.Next())

fourthNodeIndexKeyPrimary := roachpb.IndexUsageKey{
TableID: roachpb.TableID(testTableID),
IndexID: 1, // test pkey
}

fourthNodeindexKeyA := roachpb.IndexUsageKey{
TableID: roachpb.TableID(testTableID),
IndexID: 2, // secondary index
}

// Check local node stats.
// Fetch stats reader from each individual
thirdServer := testCluster.Server(2 /* idx */)
Expand All @@ -191,7 +240,14 @@ func TestStatusAPIIndexUsage(t *testing.T) {
require.Equal(t, roachpb.IndexUsageStatistics{}, stats, "expecting empty stats on node 3, but found %v", stats)

stats = thirdLocalStatsReader.Get(indexKeyB.TableID, indexKeyB.IndexID)
require.Equal(t, roachpb.IndexUsageStatistics{}, stats, "expecting empty stats on node 1, but found %v", stats)
require.Equal(t, roachpb.IndexUsageStatistics{}, stats, "expecting empty stats on node 3, but found %v", stats)

// Fourth node should have nothing - total_reads and last_read columns should not be populated.
stats = fourthLocalStatsReader.Get(fourthNodeIndexKeyPrimary.TableID, fourthNodeIndexKeyPrimary.IndexID)
require.Equal(t, roachpb.IndexUsageStatistics{}, stats, "expecting empty stats on node 4, but found %v", stats)

stats = fourthLocalStatsReader.Get(fourthNodeindexKeyA.TableID, fourthNodeindexKeyA.IndexID)
require.Equal(t, roachpb.IndexUsageStatistics{}, stats, "expecting empty stats on node 4, but found %v", stats)

// Second server should have nonempty local storage.
stats = secondLocalStatsReader.Get(indexKeyPrimary.TableID, indexKeyPrimary.IndexID)
Expand Down
11 changes: 6 additions & 5 deletions pkg/sql/opt_exec_factory.go
Expand Up @@ -123,7 +123,8 @@ func (ef *execFactory) ConstructScan(
scan.lockingWaitPolicy = descpb.ToScanLockingWaitPolicy(params.Locking.WaitPolicy)
}
scan.localityOptimized = params.LocalityOptimized
if !ef.isExplain {

if !ef.isExplain && !(ef.planner.isInternalPlanner || ef.planner.SessionData().Internal) {
idxUsageKey := roachpb.IndexUsageKey{
TableID: roachpb.TableID(tabDesc.GetID()),
IndexID: roachpb.IndexID(idx.GetID()),
Expand Down Expand Up @@ -614,7 +615,7 @@ func (ef *execFactory) ConstructIndexJoin(
tableScan.index = idx
tableScan.disableBatchLimit()

if !ef.isExplain && !ef.planner.isInternalPlanner {
if !ef.isExplain && !(ef.planner.isInternalPlanner || ef.planner.SessionData().Internal) {
idxUsageKey := roachpb.IndexUsageKey{
TableID: roachpb.TableID(tabDesc.GetID()),
IndexID: roachpb.IndexID(idx.GetID()),
Expand Down Expand Up @@ -676,7 +677,7 @@ func (ef *execFactory) ConstructLookupJoin(
tableScan.lockingWaitPolicy = descpb.ToScanLockingWaitPolicy(locking.WaitPolicy)
}

if !ef.isExplain && !ef.planner.isInternalPlanner {
if !ef.isExplain && !(ef.planner.isInternalPlanner || ef.planner.SessionData().Internal) {
idxUsageKey := roachpb.IndexUsageKey{
TableID: roachpb.TableID(tabDesc.GetID()),
IndexID: roachpb.IndexID(idx.GetID()),
Expand Down Expand Up @@ -811,7 +812,7 @@ func (ef *execFactory) ConstructInvertedJoin(
}
tableScan.index = idx

if !ef.isExplain && !ef.planner.isInternalPlanner {
if !ef.isExplain && !(ef.planner.isInternalPlanner || ef.planner.SessionData().Internal) {
idxUsageKey := roachpb.IndexUsageKey{
TableID: roachpb.TableID(tabDesc.GetID()),
IndexID: roachpb.IndexID(idx.GetID()),
Expand Down Expand Up @@ -875,7 +876,7 @@ func (ef *execFactory) constructScanForZigzag(
return nil, err
}

if !ef.isExplain && !ef.planner.isInternalPlanner {
if !ef.isExplain && !(ef.planner.isInternalPlanner || ef.planner.SessionData().Internal) {
idxUsageKey := roachpb.IndexUsageKey{
TableID: roachpb.TableID(tableDesc.GetID()),
IndexID: roachpb.IndexID(index.GetID()),
Expand Down

0 comments on commit 3c6d430

Please sign in to comment.