Merge pull request #52191 from rytaft/backport20.1-51616-51780-51805-…

…51828

release-20.1: stats: don't delete stale cache entries, update them asynchronously instead

pkg/sql/alter_table.go

@@ -1170,6 +1170,7 @@ func injectTableStats(
 	// update is handled asynchronously).
 	params.extendedEvalCtx.ExecCfg.TableStatsCache.InvalidateTableStats(params.ctx, desc.ID)
 
+	// Use Gossip to refresh the caches on other nodes.
 	return stats.GossipTableStatAdded(params.extendedEvalCtx.ExecCfg.Gossip, desc.ID)
 }
 

pkg/sql/create_stats.go

@@ -411,11 +411,6 @@ func (r *createStatsResumer) Resume(
 		return err
 	}
 
-	// Invalidate the local cache synchronously; this guarantees that the next
-	// statement in the same session won't use a stale cache (whereas the gossip
-	// update is handled asynchronously).
-	evalCtx.ExecCfg.TableStatsCache.InvalidateTableStats(ctx, r.tableID)
-
 	// Record this statistics creation in the event log.
 	if !createStatsPostEvents.Get(&evalCtx.Settings.SV) {
 		return nil

pkg/sql/opt/exec/execbuilder/testdata/stats

@@ -11,11 +11,26 @@ statement ok
 CREATE STATISTICS u ON u FROM uv;
 CREATE STATISTICS v ON v FROM uv
 
+query TTIIIB colnames
+SELECT
+  statistics_name,
+  column_names,
+  row_count,
+  distinct_count,
+  null_count,
+  histogram_id IS NOT NULL AS has_histogram
+FROM
+  [SHOW STATISTICS FOR TABLE uv]
+----
+statistics_name  column_names  row_count  distinct_count  null_count  has_histogram
+u                {u}           8          2               0           true
+v                {v}           8          7               0           true
+
 statement ok
 set enable_zigzag_join = false
 
 # Verify we scan index v which has the more selective constraint.
-query TTTTT
+query TTTTT retry
 EXPLAIN (VERBOSE) SELECT * FROM uv WHERE u = 1 AND v = 1
 ----
 ·     distributed  true         ·       ·

pkg/sql/plan_opt_test.go

@@ -88,6 +88,20 @@ func (h *queryCacheTestHelper) AssertStats(tb *testing.T, expHits, expMisses int
 	assert.Equal(tb, expMisses, misses, "misses")
 }
 
+// CheckStats is similar to AssertStats, but returns an error instead of
+// failing the test if the actual stats don't match the expected stats.
+func (h *queryCacheTestHelper) CheckStats(tb *testing.T, expHits, expMisses int) error {
+	tb.Helper()
+	hits, misses := h.GetStats()
+	if expHits != hits {
+		return errors.Errorf("expected %d hits but found %d", expHits, hits)
+	}
+	if expMisses != misses {
+		return errors.Errorf("expected %d misses but found %d", expMisses, misses)
+	}
+	return nil
+}
+
 func TestQueryCache(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 
@@ -328,8 +342,17 @@ SELECT cte.x, cte.y FROM cte LEFT JOIN cte as cte2 on cte.y = cte2.x`, j)
 			h.AssertStats(t, 1 /* hits */, 1 /* misses */)
 			r0.Exec(t, "CREATE STATISTICS s FROM t")
 			h.AssertStats(t, 1 /* hits */, 1 /* misses */)
-			r1.CheckQueryResults(t, "SELECT * FROM t", [][]string{{"1", "1"}})
-			h.AssertStats(t, 1 /* hits */, 2 /* misses */)
+			hits := 1
+			testutils.SucceedsSoon(t, func() error {
+				// The stats cache is updated asynchronously, so we may get some hits
+				// before we get a miss.
+				r1.CheckQueryResults(t, "SELECT * FROM t", [][]string{{"1", "1"}})
+				if err := h.CheckStats(t, hits, 2 /* misses */); err != nil {
+					hits++
+					return err
+				}
+				return nil
+			})
 		})
 
 		// Test that a schema change triggers cache invalidation.

pkg/sql/rowexec/sample_aggregator.go

@@ -388,7 +388,7 @@ func (s *sampleAggregator) writeResults(ctx context.Context) error {
 		return err
 	}
 
-	// Gossip invalidation of the stat caches for this table.
+	// Gossip refresh of the stat caches for this table.
 	return stats.GossipTableStatAdded(s.FlowCtx.Cfg.Gossip, s.tableID)
 }
 

pkg/sql/stats/automatic_stats_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
+	"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/util/leaktest"
@@ -128,41 +129,45 @@ func TestAverageRefreshTime(t *testing.T) {
 	refresher := MakeRefresher(st, executor, cache, time.Microsecond /* asOfTime */)
 
 	checkAverageRefreshTime := func(expected time.Duration) error {
-		cache.InvalidateTableStats(ctx, tableID)
-		stats, err := cache.GetTableStats(ctx, tableID)
-		if err != nil {
-			return err
-		}
-		if actual := avgRefreshTime(stats).Round(time.Minute); actual != expected {
-			return fmt.Errorf("expected avgRefreshTime %s but found %s",
-				expected.String(), actual.String())
-		}
-		return nil
+		cache.RefreshTableStats(ctx, tableID)
+		return testutils.SucceedsSoonError(func() error {
+			stats, err := cache.GetTableStats(ctx, tableID)
+			if err != nil {
+				return err
+			}
+			if actual := avgRefreshTime(stats).Round(time.Minute); actual != expected {
+				return fmt.Errorf("expected avgRefreshTime %s but found %s",
+					expected.String(), actual.String())
+			}
+			return nil
+		})
 	}
 
 	// Checks that the most recent statistic was created less than (greater than)
 	// expectedAge time ago if lessThan is true (false).
 	checkMostRecentStat := func(expectedAge time.Duration, lessThan bool) error {
-		cache.InvalidateTableStats(ctx, tableID)
-		stats, err := cache.GetTableStats(ctx, tableID)
-		if err != nil {
-			return err
-		}
-		stat := mostRecentAutomaticStat(stats)
-		if stat == nil {
-			return fmt.Errorf("no recent automatic statistic found")
-		}
-		if !lessThan && stat.CreatedAt.After(timeutil.Now().Add(-1*expectedAge)) {
-			return fmt.Errorf("most recent stat is less than %s old. Created at: %s Current time: %s",
-				expectedAge, stat.CreatedAt, timeutil.Now(),
-			)
-		}
-		if lessThan && stat.CreatedAt.Before(timeutil.Now().Add(-1*expectedAge)) {
-			return fmt.Errorf("most recent stat is more than %s old. Created at: %s Current time: %s",
-				expectedAge, stat.CreatedAt, timeutil.Now(),
-			)
-		}
-		return nil
+		cache.RefreshTableStats(ctx, tableID)
+		return testutils.SucceedsSoonError(func() error {
+			stats, err := cache.GetTableStats(ctx, tableID)
+			if err != nil {
+				return err
+			}
+			stat := mostRecentAutomaticStat(stats)
+			if stat == nil {
+				return fmt.Errorf("no recent automatic statistic found")
+			}
+			if !lessThan && stat.CreatedAt.After(timeutil.Now().Add(-1*expectedAge)) {
+				return fmt.Errorf("most recent stat is less than %s old. Created at: %s Current time: %s",
+					expectedAge, stat.CreatedAt, timeutil.Now(),
+				)
+			}
+			if lessThan && stat.CreatedAt.Before(timeutil.Now().Add(-1*expectedAge)) {
+				return fmt.Errorf("most recent stat is more than %s old. Created at: %s Current time: %s",
+					expectedAge, stat.CreatedAt, timeutil.Now(),
+				)
+			}
+			return nil
+		})
 	}
 
 	// Since there are no stats yet, avgRefreshTime should return the default
@@ -446,13 +451,15 @@ func TestDefaultColumns(t *testing.T) {
 func checkStatsCount(
 	ctx context.Context, cache *TableStatisticsCache, tableID sqlbase.ID, expected int,
 ) error {
-	cache.InvalidateTableStats(ctx, tableID)
-	stats, err := cache.GetTableStats(ctx, tableID)
-	if err != nil {
-		return err
-	}
-	if len(stats) != expected {
-		return fmt.Errorf("expected %d stat(s) but found %d", expected, len(stats))
-	}
-	return nil
+	cache.RefreshTableStats(ctx, tableID)
+	return testutils.SucceedsSoonError(func() error {
+		stats, err := cache.GetTableStats(ctx, tableID)
+		if err != nil {
+			return err
+		}
+		if len(stats) != expected {
+			return fmt.Errorf("expected %d stat(s) but found %d", expected, len(stats))
+		}
+		return nil
+	})
 }

pkg/sql/stats/delete_stats_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -257,37 +258,45 @@ func TestDeleteOldStatsForColumns(t *testing.T) {
 			return err
 		}
 
-		cache.InvalidateTableStats(ctx, tableID)
-		tableStats, err := cache.GetTableStats(ctx, tableID)
-		if err != nil {
-			return err
-		}
-
+		cache.RefreshTableStats(ctx, tableID)
 		for i := range testData {
 			stat := &testData[i]
 			if stat.TableID != tableID {
-				cache.InvalidateTableStats(ctx, stat.TableID)
-				stats, err := cache.GetTableStats(ctx, stat.TableID)
-				if err != nil {
-					return err
-				}
-				// No stats from other tables should be deleted.
-				if err := findStat(
-					stats, stat.TableID, stat.StatisticID, false, /* expectDeleted */
-				); err != nil {
-					return err
-				}
-				continue
+				cache.RefreshTableStats(ctx, stat.TableID)
 			}
+		}
 
-			// Check whether this stat should have been deleted.
-			_, expectDeleted := expectDeleted[stat.StatisticID]
-			if err := findStat(tableStats, tableID, stat.StatisticID, expectDeleted); err != nil {
+		return testutils.SucceedsSoonError(func() error {
+			tableStats, err := cache.GetTableStats(ctx, tableID)
+			if err != nil {
 				return err
 			}
-		}
 
-		return nil
+			for i := range testData {
+				stat := &testData[i]
+				if stat.TableID != tableID {
+					stats, err := cache.GetTableStats(ctx, stat.TableID)
+					if err != nil {
+						return err
+					}
+					// No stats from other tables should be deleted.
+					if err := findStat(
+						stats, stat.TableID, stat.StatisticID, false, /* expectDeleted */
+					); err != nil {
+						return err
+					}
+					continue
+				}
+
+				// Check whether this stat should have been deleted.
+				_, expectDeleted := expectDeleted[stat.StatisticID]
+				if err := findStat(tableStats, tableID, stat.StatisticID, expectDeleted); err != nil {
+					return err
+				}
+			}
+
+			return nil
+		})
 	}
 
 	expectDeleted := make(map[uint64]struct{}, len(testData))