sql/hints: add statement hint cache generation

This commit adds a generation to the statement hints cache, which is
incremented every time the cache processes a rangefeed event. The generation
is used as part of the `cat.DependencyDigest`, used to decide whether an
expensive staleness check is necessary for a cached plan.

Informs #148160

Release note: None

Author: DrewKimball

pkg/sql/hints/hint_cache.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"hash/fnv"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
@@ -87,6 +88,10 @@ type StatementHintsCache struct {
 
 	// Used to read hints from the database when they are not in the cache.
 	db descs.DB
+
+	// generation is incremented any time the hint cache is updated by the
+	// rangefeed.
+	generation atomic.Int64
 }
 
 // cacheSize is the size of the entries to store in the cache.
@@ -214,6 +219,7 @@ func (c *StatementHintsCache) onUpdate(
 // handleInitialScan builds the hintedHashes map and adds it to
 // StatementHintsCache after the initial scan completes.
 func (c *StatementHintsCache) handleInitialScan(update rangefeedcache.Update[*bufferEvent]) {
+	defer c.generation.Add(1)
 	hintedHashes := make(map[int64]hlc.Timestamp)
 	for _, ev := range update.Events {
 		if ev.del {
@@ -246,6 +252,7 @@ func (c *StatementHintsCache) handleIncrementalUpdate(
 		// Avoid synchronization when we're just bumping the resolved timestamp.
 		return
 	}
+	defer c.generation.Add(1)
 	c.mu.Lock()
 	defer c.mu.Unlock()
 	for _, ev := range update.Events {
@@ -309,8 +316,6 @@ func (c *StatementHintsCache) checkHashHasHintsAsync(
 					// Unset the timestamp to indicate that the refresh is done.
 					c.mu.hintedHashes[hash] = hlc.Timestamp{}
 				} else {
-					// There may be a hintCache entry with no hints; drop it.
-					c.mu.hintCache.Del(hash)
 					delete(c.mu.hintedHashes, hash)
 				}
 				return true
@@ -378,6 +383,12 @@ var _ rangefeedbuffer.Event = &bufferEvent{}
 // Reading from the cache.
 // ============================================================================
 
+// GetGeneration returns the current generation, which will change if any
+// modifications happen to the cache.
+func (c *StatementHintsCache) GetGeneration() int64 {
+	return c.generation.Load()
+}
+
 // MaybeGetStatementHints attempts to retrieve the hints for the given statement
 // fingerprint. It returns nil if the statement has no hints, or there was an
 // error retrieving them.

pkg/sql/hints/hint_cache_test.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"hash/fnv"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangefeed"
@@ -21,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 )
@@ -372,6 +374,98 @@ func TestHintCacheMultiTenant(t *testing.T) {
 	require.Nil(t, hc2.MaybeGetStatementHints(ctx, fingerprintShared))
 }
 
+// TestHintCacheGeneration tests that the cache generation is incremented
+// correctly when hints are added and removed, and that the generation is not
+// incremented when there are no updates.
+func TestHintCacheGeneration(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	rnd, _ := randutil.NewTestRand()
+	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
+	defer srv.Stopper().Stop(ctx)
+	ts := srv.ApplicationLayer()
+	r := sqlutils.MakeSQLRunner(db)
+	setTestDefaults(t, srv)
+
+	// Create a hints cache.
+	hc := createHintsCache(t, ctx, ts)
+
+	// Helper that retrieves the generation and verifies that it doesn't change
+	// over a short period.
+	getGenerationAssertNoChange := func() int64 {
+		t.Helper()
+		startGeneration := hc.GetGeneration()
+		time.Sleep(time.Duration(rnd.Intn(int(time.Millisecond))))
+		require.Equal(t, startGeneration, hc.GetGeneration())
+		return startGeneration
+	}
+	// Helper that waits until the generation increments from the given start
+	// point.
+	waitForGenerationInc := func(prevGeneration int64) {
+		t.Helper()
+		testutils.SucceedsSoon(t, func() error {
+			t.Helper()
+			if gen := hc.GetGeneration(); gen <= prevGeneration {
+				return errors.Errorf("expected generation >= %d, got %d", prevGeneration, gen)
+			}
+			return nil
+		})
+	}
+
+	// The initial scan should increment the generation.
+	waitForGenerationInc(0)
+	generationAfterInitialScan := getGenerationAssertNoChange()
+
+	// Insert a hint - generation should increment.
+	fingerprint1 := "SELECT a FROM t WHERE b = $1"
+	insertStatementHint(t, r, fingerprint1)
+	waitForGenerationInc(generationAfterInitialScan)
+	waitForUpdateOnFingerprintHash(t, ctx, hc, fingerprint1, 1)
+	generationAfterInsert := getGenerationAssertNoChange()
+
+	// Insert another hint for the same fingerprint - generation should increment
+	// again.
+	insertStatementHint(t, r, fingerprint1)
+	waitForGenerationInc(generationAfterInsert)
+	waitForUpdateOnFingerprintHash(t, ctx, hc, fingerprint1, 2)
+	generationAfterSecondInsert := getGenerationAssertNoChange()
+
+	// Add a hint for a different fingerprint - generation should increment.
+	fingerprint2 := "SELECT c FROM t WHERE d = $1"
+	insertStatementHint(t, r, fingerprint2)
+	waitForGenerationInc(generationAfterSecondInsert)
+	waitForUpdateOnFingerprintHash(t, ctx, hc, fingerprint2, 1)
+	generationAfterDifferentFingerprint := getGenerationAssertNoChange()
+
+	// Delete one hint - generation should increment.
+	deleteStatementHints(t, r, fingerprint1, 1)
+	waitForGenerationInc(generationAfterDifferentFingerprint)
+	waitForUpdateOnFingerprintHash(t, ctx, hc, fingerprint1, 1)
+	generationAfterDelete := getGenerationAssertNoChange()
+
+	// Delete all remaining hints for fingerprint1 - generation should increment.
+	deleteStatementHints(t, r, fingerprint1, 0)
+	waitForGenerationInc(generationAfterDelete)
+	waitForUpdateOnFingerprintHash(t, ctx, hc, fingerprint1, 0)
+	generationAfterDeleteAll := getGenerationAssertNoChange()
+
+	// Query for hints (cache access) should NOT increment generation.
+	hc.MaybeGetStatementHints(ctx, fingerprint2)
+	getGenerationAssertNoChange()
+
+	// Accessing a non-existent fingerprint should also NOT increment generation.
+	hc.MaybeGetStatementHints(ctx, "SELECT nonexistent FROM t")
+	getGenerationAssertNoChange()
+
+	// Delete all remaining hints.
+	deleteStatementHints(t, r, fingerprint2, 0)
+	waitForGenerationInc(generationAfterDeleteAll)
+	waitForUpdateOnFingerprintHash(t, ctx, hc, fingerprint2, 0)
+	getGenerationAssertNoChange()
+}
+
 func setTestDefaults(t *testing.T, srv serverutils.TestServerInterface) {
 	// These settings can only be set on the system tenant.
 	r := sqlutils.MakeSQLRunner(srv.SystemLayer().SQLConn(t))

pkg/sql/opt/cat/catalog.go

@@ -95,6 +95,10 @@ type DependencyDigest struct {
 	// StatsGeneration tracks if any new statistics have been published.
 	StatsGeneration int64
 
+	// HintsGeneration tracks if any new hints have been published in
+	// system.statement_hints.
+	HintsGeneration int64
+
 	// SystemConfig tracks the current system config, which is refreshed on
 	// any zone config update.
 	SystemConfig *config.SystemConfig
@@ -108,6 +112,7 @@ type DependencyDigest struct {
 func (d *DependencyDigest) Equal(other *DependencyDigest) bool {
 	return d.LeaseGeneration == other.LeaseGeneration &&
 		d.StatsGeneration == other.StatsGeneration &&
+		d.HintsGeneration == other.HintsGeneration &&
 		// Note: If the system config is modified a new SystemConfig structure
 		// is always allocated. Individual fields cannot change on the caller,
 		// so for the purpose of the dependency digest its sufficient to just

pkg/sql/opt_catalog.go

@@ -603,15 +603,17 @@ func (oc *optCatalog) LeaseByStableID(ctx context.Context, stableID cat.StableID
 
 // GetDependencyDigest is part of the cat.Catalog interface.
 func (oc *optCatalog) GetDependencyDigest() cat.DependencyDigest {
-	// The stats cache may not be setup in some tests like
+	// The stats and hints caches may not be setup in some tests like
 	// TestPortalsDestroyedOnTxnFinish. In which case always
 	// return the empty digest.
-	if oc.planner.ExecCfg().TableStatsCache == nil {
+	if oc.planner.ExecCfg().TableStatsCache == nil ||
+		oc.planner.ExecCfg().StatementHintsCache == nil {
 		return cat.DependencyDigest{}
 	}
 	return cat.DependencyDigest{
 		LeaseGeneration: oc.planner.Descriptors().GetLeaseGeneration(),
 		StatsGeneration: oc.planner.execCfg.TableStatsCache.GetGeneration(),
+		HintsGeneration: oc.planner.execCfg.StatementHintsCache.GetGeneration(),
 		SystemConfig:    oc.planner.execCfg.SystemConfig.GetSystemConfig(),
 		CurrentDatabase: oc.planner.CurrentDatabase(),
 		SearchPath:      oc.planner.SessionData().SearchPath,

pkg/sql/tests/dependency_digest_test.go

@@ -21,6 +21,7 @@ import (
 
 // TestDependencyDigestOptimization validates that dependency digest information
 // is properly invalidated in the face of modifications for prepared queries.
+// TODO(drewk,michae2): Once statement hints are hooked up, add a case for them.
 func TestDependencyDigestOptimization(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)