Merge cockroachdb#142963

142963: sql/stats: unset histogram buckets proto after decoding r=yuzefovich a=yuzefovich

**sql/stats,backup: adjust table stats handling in backup**

This commit introduces a new method to return the protos of table
statistics directly from the DB (i.e. it by-passes the table stats cache)
and uses this method in the backup code. The main motivation is that we
want to nil out the histogram buckets in the proto that we store in the
table stats cache since we don't need them outside of the backup.

The new method hardens the logic of excluding merged and forecast stats
from the backup because we simply no longer include these stats into
what backups includes (because they are generated on demand rather than
read from the system table). It additionally ignores whether "stats
usage is allowed" on a particular table since it seems beneficial to
backup stats for all requested tables, and then once stats are restored
we can decide whether to actually use them.

Notable change for the new `GetTableStatsProtosFromDB` method is that we
no longer hydrate the column type nor decode upper bounds since this isn't
needed for the backup use case.

**stats: unset histogram buckets proto after decoding**

I think it should be safe to unset `HistogramData.Buckets` proto after
having decoded the histogram buckets since we only use `cat.HistogramBucket`
with decoded datums going forward (the only exception was the backup use
case, but it was adjusted in the previous commit to have a separate code
path). I noticed the proto was showing up noticeably in the inuse-space
heap profiles on a customer cluster.

It also adds a comment for why we're use `*bool` as `forecast` argument
in `getTableStatsFromCache`, as opposed to just `bool`.

Epic: None
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>

Author: craig[bot]

pkg/backup/backup_compaction.go

@@ -879,7 +879,7 @@ func concludeBackupCompaction(
 		}
 	}
 
-	statsTable := getTableStatsForBackup(ctx, execCtx.ExecCfg().TableStatsCache, backupManifest.Descriptors)
+	statsTable := getTableStatsForBackup(ctx, execCtx.ExecCfg().InternalDB.Executor(), backupManifest.Descriptors)
 	return backupinfo.WriteTableStatistics(ctx, store, encryption, kmsEnv, &statsTable)
 }
 

pkg/backup/backup_job.go

@@ -135,7 +135,6 @@ func backup(
 	backupManifest *backuppb.BackupManifest,
 	makeExternalStorage cloud.ExternalStorageFactory,
 	encryption *jobspb.BackupEncryptionOptions,
-	statsCache *stats.TableStatisticsCache,
 	execLocality roachpb.Locality,
 ) (_ roachpb.RowCount, numBackupInstances int, _ error) {
 	resumerSpan := tracing.SpanFromContext(ctx)
@@ -435,7 +434,7 @@ func backup(
 		}
 	}
 
-	statsTable := getTableStatsForBackup(ctx, statsCache, backupManifest.Descriptors)
+	statsTable := getTableStatsForBackup(ctx, execCtx.ExecCfg().InternalDB.Executor(), backupManifest.Descriptors)
 	if err := backupinfo.WriteTableStatistics(ctx, defaultStore, encryption, &kmsEnv, &statsTable); err != nil {
 		return roachpb.RowCount{}, 0, err
 	}
@@ -468,25 +467,24 @@ func releaseProtectedTimestamp(
 // to suboptimal performance when reading/writing to this table until
 // the stats have been recomputed.
 func getTableStatsForBackup(
-	ctx context.Context, statsCache *stats.TableStatisticsCache, descs []descpb.Descriptor,
+	ctx context.Context, executor isql.Executor, descs []descpb.Descriptor,
 ) backuppb.StatsTable {
 	var tableStatistics []*stats.TableStatisticProto
 	for i := range descs {
 		if tbl, _, _, _, _ := descpb.GetDescriptors(&descs[i]); tbl != nil {
 			tableDesc := tabledesc.NewBuilder(tbl).BuildImmutableTable()
-			// nil typeResolver means that we'll use the latest committed type
-			// metadata which is acceptable.
-			tableStatisticsAcc, err := statsCache.GetTableStats(ctx, tableDesc, nil /* typeResolver */)
+			tableStatisticsAcc, err := stats.GetTableStatsProtosFromDB(ctx, tableDesc, executor)
 			if err != nil {
-				log.Warningf(ctx, "failed to collect stats for table: %s, "+
-					"table ID: %d during a backup: %s", tableDesc.GetName(), tableDesc.GetID(),
-					err.Error())
+				log.Warningf(
+					ctx, "failed to collect stats for table: %s, table ID: %d during a backup: %s",
+					tableDesc.GetName(), tableDesc.GetID(), err,
+				)
 				continue
 			}
 
 			for _, stat := range tableStatisticsAcc {
-				if statShouldBeIncludedInBackupRestore(&stat.TableStatisticProto) {
-					tableStatistics = append(tableStatistics, &stat.TableStatisticProto)
+				if statShouldBeIncludedInBackupRestore(stat) {
+					tableStatistics = append(tableStatistics, stat)
 				}
 			}
 		}
@@ -699,7 +697,6 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 		}
 	}
 
-	statsCache := p.ExecCfg().TableStatsCache
 	// We retry on pretty generic failures -- any rpc error. If a worker node were
 	// to restart, it would produce this kind of error, but there may be other
 	// errors that are also rpc errors. Don't retry too aggressively.
@@ -735,7 +732,6 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 			backupManifest,
 			p.ExecCfg().DistSQLSrv.ExternalStorage,
 			details.EncryptionOptions,
-			statsCache,
 			details.ExecutionLocality,
 		)
 		if err == nil {

pkg/sql/stats/automatic_stats.go

@@ -887,7 +887,11 @@ func (r *Refresher) maybeRefreshStats(
 	partialStatsEnabled bool,
 	fullStatsEnabled bool,
 ) {
-	tableStats, err := r.cache.getTableStatsFromCache(ctx, tableID, nil /* forecast */, nil /* udtCols */, nil /* typeResolver */)
+	// NB: we pass nil boolean as 'forecast' argument in order to not invalidate
+	// the stats cache entry since we don't care whether there is a forecast or
+	// not in the stats.
+	var forecast *bool
+	tableStats, err := r.cache.getTableStatsFromCache(ctx, tableID, forecast, nil /* udtCols */, nil /* typeResolver */)
 	if err != nil {
 		log.Errorf(ctx, "failed to get table statistics: %v", err)
 		return

pkg/sql/stats/stats_cache.go

@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/rowenc"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -47,15 +48,9 @@ type TableStatistic struct {
 	Histogram []cat.HistogramBucket
 }
 
-// A TableStatisticsCache contains two underlying LRU caches:
-// (1) A cache of []*TableStatistic objects, keyed by table ID.
-//
-//	Each entry consists of all the statistics for different columns and
-//	column groups for the given table.
-//
-// (2) A cache of *HistogramData objects, keyed by
-//
-//	HistogramCacheKey{table ID, statistic ID}.
+// A TableStatisticsCache contains an LRU cache of []*TableStatistic objects,
+// keyed by table ID. Each entry consists of all the statistics for different
+// columns and column groups for the given table.
 type TableStatisticsCache struct {
 	// NB: This can't be a RWMutex for lookup because UnorderedCache.Get
 	// manipulates an internal LRU list.
@@ -232,7 +227,7 @@ func decodeTableStatisticsKV(
 //
 // The function returns an error if we could not query the system table. It
 // silently ignores any statistics that can't be decoded (e.g. because
-// user-defined types don't exit).
+// user-defined types don't exist).
 //
 // The statistics are ordered by their CreatedAt time (newest-to-oldest).
 func (sc *TableStatisticsCache) GetTableStats(
@@ -247,6 +242,23 @@ func (sc *TableStatisticsCache) GetTableStats(
 	)
 }
 
+// GetTableStatsProtosFromDB looks up statistics for the requested table in
+// system.table_statistics, by-passing the cache. Merged (based on partial and
+// latest full) and forecast stats are **not** included in the result.
+//
+// NB: the ColumnType field of HistogramData is not hydrated.
+//
+// The function returns an error if we could not query the system table. It
+// silently ignores any statistics that can't be decoded (e.g. because
+// user-defined types don't exist).
+//
+// The statistics are ordered by their CreatedAt time (newest-to-oldest).
+func GetTableStatsProtosFromDB(
+	ctx context.Context, table catalog.TableDescriptor, executor isql.Executor,
+) (statsProtos []*TableStatisticProto, err error) {
+	return getTableStatsProtosFromDB(ctx, table.GetID(), executor)
+}
+
 // DisallowedOnSystemTable returns true if this tableID belongs to a special
 // system table on which we want to disallow stats collection and stats usage.
 func DisallowedOnSystemTable(tableID descpb.ID) bool {
@@ -705,6 +717,9 @@ func (sc *TableStatisticsCache) parseStats(
 		if err = DecodeHistogramBuckets(res); err != nil {
 			return nil, nil, err
 		}
+		// Update the HistogramData proto to nil out Buckets field to allow for
+		// the memory to be GCed.
+		res.HistogramData.Buckets = nil
 	}
 	return res, udt, nil
 }
@@ -810,19 +825,9 @@ func (tsp *TableStatisticProto) IsAuto() bool {
 	return tsp.Name == jobspb.AutoStatsName
 }
 
-// getTableStatsFromDB retrieves the statistics in system.table_statistics
-// for the given table ID.
-//
-// It ignores any statistics that cannot be decoded (e.g. because a user-defined
-// type that doesn't exist) and returns the rest (with no error).
-func (sc *TableStatisticsCache) getTableStatsFromDB(
-	ctx context.Context,
-	tableID descpb.ID,
-	forecast bool,
-	st *cluster.Settings,
-	typeResolver *descs.DistSQLTypeResolver,
-) (_ []*TableStatistic, _ map[descpb.ColumnID]*types.T, err error) {
-	getTableStatisticsStmt := `
+// TODO(michae2): Add an index on system.table_statistics (tableID, createdAt,
+// columnIDs, statisticID).
+const getTableStatisticsStmt = `
 SELECT
 	"tableID",
 	"statisticID",
@@ -840,9 +845,19 @@ FROM system.table_statistics
 WHERE "tableID" = $1
 ORDER BY "createdAt" DESC, "columnIDs" DESC, "statisticID" DESC
 `
-	// TODO(michae2): Add an index on system.table_statistics (tableID, createdAt,
-	// columnIDs, statisticID).
 
+// getTableStatsFromDB retrieves the statistics in system.table_statistics
+// for the given table ID.
+//
+// It ignores any statistics that cannot be decoded (e.g. because of a
+// user-defined type that doesn't exist) and returns the rest (with no error).
+func (sc *TableStatisticsCache) getTableStatsFromDB(
+	ctx context.Context,
+	tableID descpb.ID,
+	forecast bool,
+	st *cluster.Settings,
+	typeResolver *descs.DistSQLTypeResolver,
+) (_ []*TableStatistic, _ map[descpb.ColumnID]*types.T, err error) {
 	it, err := sc.db.Executor().QueryIteratorEx(
 		ctx, "get-table-statistics", nil /* txn */, sessiondata.NodeUserSessionDataOverride, getTableStatisticsStmt, tableID,
 	)
@@ -912,3 +927,48 @@ ORDER BY "createdAt" DESC, "columnIDs" DESC, "statisticID" DESC
 
 	return statsList, udts, nil
 }
+
+// getTableStatsProtosFromDB retrieves the statistics in system.table_statistics
+// for the given table ID, in "un-decoded" protobuf form.
+//
+// It ignores any statistics that cannot be decoded (e.g. because of a
+// user-defined type that doesn't exist) and returns the rest (with no error).
+func getTableStatsProtosFromDB(
+	ctx context.Context, tableID descpb.ID, executor isql.Executor,
+) (statsProtos []*TableStatisticProto, err error) {
+	it, err := executor.QueryIteratorEx(
+		ctx, "get-table-statistics-protos", nil /* txn */, sessiondata.NodeUserSessionDataOverride, getTableStatisticsStmt, tableID,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	// Guard against crashes in the code below.
+	defer func() {
+		if r := recover(); r != nil {
+			// In the event of a "safe" panic, we only want to log the error and
+			// continue executing the query without stats for this table. This is only
+			// possible because the code does not update shared state and does not
+			// manipulate locks.
+			if ok, e := errorutil.ShouldCatch(r); ok {
+				err = e
+			} else {
+				// Other panic objects can't be considered "safe" and thus are
+				// propagated as crashes that terminate the session.
+				panic(r)
+			}
+		}
+	}()
+
+	var ok bool
+	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
+		var tsp *TableStatisticProto
+		tsp, err = NewTableStatisticProto(it.Cur())
+		if err != nil {
+			log.Warningf(ctx, "could not decode statistic for table %d: %v", tableID, err)
+			continue
+		}
+		statsProtos = append(statsProtos, tsp)
+	}
+	return statsProtos, nil
+}

pkg/sql/stats/stats_cache_test.go

@@ -197,6 +197,10 @@ func initTestData(
 			return nil, err
 		}
 
+		if stat.HistogramData != nil {
+			// Match the behavior from TableStatisticsCache.parseStats.
+			stat.HistogramData.Buckets = nil
+		}
 		expectedStats[stat.TableID] = append(expectedStats[stat.TableID], stat)
 	}