stats: use leased types in the stats cache

pkg/server/server_sql.go

@@ -458,6 +458,8 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) {
 			cfg.db,
 			cfg.circularInternalExecutor,
 			codec,
+			leaseMgr,
+			cfg.Settings,
 		),
 
 		// Note: don't forget to add the secondary loggers as closers

pkg/sql/catalog/resolver/resolver.go

@@ -16,7 +16,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -274,55 +273,6 @@ func ResolveSchemaNameByID(
 	return "", errors.Newf("unable to resolve schema id %d for db %d", schemaID, dbID)
 }
 
-// ResolveTypeDescByID resolves a TypeDescriptor and fully qualified name
-// from an ID.
-// TODO (rohany): Once we start to cache type descriptors, this needs to
-//  look into the set of leased copies.
-// TODO (rohany): Once we lease types, this should be pushed down into the
-//  leased object collection.
-func ResolveTypeDescByID(
-	ctx context.Context,
-	txn *kv.Txn,
-	codec keys.SQLCodec,
-	id descpb.ID,
-	lookupFlags tree.ObjectLookupFlags,
-) (tree.TypeName, sqlbase.TypeDescriptor, error) {
-	desc, err := catalogkv.GetDescriptorByID(ctx, txn, codec, id, catalogkv.Immutable,
-		catalogkv.TypeDescriptorKind, lookupFlags.Required)
-	if err != nil {
-		if pgerror.GetPGCode(err) == pgcode.WrongObjectType {
-			err = errors.HandleAsAssertionFailure(err)
-		}
-		return tree.TypeName{}, nil, err
-	}
-	// Get the parent database and schema names to create a fully qualified
-	// name for the type.
-	// TODO (SQLSchema): As we add leasing for all descriptors, these calls
-	//  should look into those leased copies, rather than do raw reads.
-	typDesc := desc.(*sqlbase.ImmutableTypeDescriptor)
-	db, err := catalogkv.MustGetDatabaseDescByID(ctx, txn, codec, typDesc.ParentID)
-	if err != nil {
-		return tree.TypeName{}, nil, err
-	}
-	schemaName, err := ResolveSchemaNameByID(ctx, txn, codec, typDesc.ParentID, typDesc.ParentSchemaID)
-	if err != nil {
-		return tree.TypeName{}, nil, err
-	}
-	name := tree.MakeNewQualifiedTypeName(db.GetName(), schemaName, typDesc.GetName())
-	var ret sqlbase.TypeDescriptor
-	if lookupFlags.RequireMutable {
-		// TODO(ajwerner): Figure this out later when we construct this inside of
-		// the name resolution. This really shouldn't be happening here. Instead we
-		// should be taking a SchemaResolver and resolving through it which should
-		// be able to hit a descs.Collection and determine whether this is a new
-		// type or not.
-		desc = sqlbase.NewMutableExistingTypeDescriptor(*typDesc.TypeDesc())
-	} else {
-		ret = typDesc
-	}
-	return name, ret, nil
-}
-
 // GetForDatabase looks up and returns all available
 // schema ids to names for a given database.
 func GetForDatabase(

pkg/sql/stats/automatic_stats_test.go

@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -65,6 +66,8 @@ func TestMaybeRefreshStats(t *testing.T) {
 		kvDB,
 		executor,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	refresher := MakeRefresher(st, executor, cache, time.Microsecond /* asOfTime */)
 
@@ -142,6 +145,8 @@ func TestAverageRefreshTime(t *testing.T) {
 		kvDB,
 		executor,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	refresher := MakeRefresher(st, executor, cache, time.Microsecond /* asOfTime */)
 
@@ -378,6 +383,8 @@ func TestAutoStatsReadOnlyTables(t *testing.T) {
 		kvDB,
 		executor,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	refresher := MakeRefresher(st, executor, cache, time.Microsecond /* asOfTime */)
 
@@ -416,6 +423,8 @@ func TestNoRetryOnFailure(t *testing.T) {
 		kvDB,
 		executor,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	r := MakeRefresher(st, executor, cache, time.Microsecond /* asOfTime */)
 

pkg/sql/stats/delete_stats_test.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
@@ -44,6 +45,8 @@ func TestDeleteOldStatsForColumns(t *testing.T) {
 		db,
 		ex,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 
 	// The test data must be ordered by CreatedAt DESC so the calculated set of

pkg/sql/stats/gossip_invalidation_test.go

@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -38,12 +39,15 @@ func TestGossipInvalidation(t *testing.T) {
 	tc := testcluster.StartTestCluster(t, 3, base.TestClusterArgs{})
 	defer tc.Stopper().Stop(ctx)
 
+	s := tc.Server(0)
 	sc := stats.NewTableStatisticsCache(
 		10, /* cacheSize */
-		gossip.MakeOptionalGossip(tc.Server(0).GossipI().(*gossip.Gossip)),
-		tc.Server(0).DB(),
-		tc.Server(0).InternalExecutor().(sqlutil.InternalExecutor),
+		gossip.MakeOptionalGossip(s.GossipI().(*gossip.Gossip)),
+		s.DB(),
+		s.InternalExecutor().(sqlutil.InternalExecutor),
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 
 	sr0 := sqlutils.MakeSQLRunner(tc.ServerConn(0))

pkg/sql/stats/stats_cache.go

@@ -18,8 +18,10 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlbase"
@@ -61,6 +63,9 @@ type TableStatisticsCache struct {
 	ClientDB    *kv.DB
 	SQLExecutor sqlutil.InternalExecutor
 	Codec       keys.SQLCodec
+
+	LeaseMgr *lease.Manager
+	Settings *cluster.Settings
 }
 
 // The cache stores *cacheEntry objects. The fields are protected by the
@@ -99,11 +104,15 @@ func NewTableStatisticsCache(
 	db *kv.DB,
 	sqlExecutor sqlutil.InternalExecutor,
 	codec keys.SQLCodec,
+	leaseManager *lease.Manager,
+	settings *cluster.Settings,
 ) *TableStatisticsCache {
 	tableStatsCache := &TableStatisticsCache{
 		ClientDB:    db,
 		SQLExecutor: sqlExecutor,
 		Codec:       codec,
+		LeaseMgr:    leaseManager,
+		Settings:    settings,
 	}
 	tableStatsCache.mu.cache = cache.NewUnorderedCache(cache.Config{
 		Policy:      cache.CacheLRU,
@@ -334,8 +343,8 @@ const (
 
 // parseStats converts the given datums to a TableStatistic object. It might
 // need to run a query to get user defined type metadata.
-func parseStats(
-	ctx context.Context, db *kv.DB, codec keys.SQLCodec, datums tree.Datums,
+func (sc *TableStatisticsCache) parseStats(
+	ctx context.Context, datums tree.Datums,
 ) (*TableStatistic, error) {
 	if datums == nil || datums.Len() == 0 {
 		return nil, nil
@@ -401,28 +410,25 @@ func parseStats(
 
 		// Decode the histogram data so that it's usable by the opt catalog.
 		res.Histogram = make([]cat.HistogramBucket, len(res.HistogramData.Buckets))
-		typ := res.HistogramData.ColumnType
 		// Hydrate the type in case any user defined types are present.
 		// There are cases where typ is nil, so don't do anything if so.
-		if typ != nil && typ.UserDefined() {
-			// TODO (rohany): This should instead query a leased copy of the type.
-			// TODO (rohany): If we are caching data about types here, then this
-			//  cache needs to be invalidated as well when type metadata changes.
-			// TODO (rohany): It might be better to store the type metadata used when
-			//  collecting the stats in the HistogramData object itself, and avoid
-			//  this query and caching/leasing problem.
+		if typ := res.HistogramData.ColumnType; typ != nil && typ.UserDefined() {
 			// The metadata accessed here is never older than the metadata used when
 			// collecting the stats. Changes to types are backwards compatible across
 			// versions, so using a newer version of the type metadata here is safe.
-			err := db.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-				typeLookup := func(ctx context.Context, id descpb.ID) (tree.TypeName, sqlbase.TypeDescriptor, error) {
-					return resolver.ResolveTypeDescByID(ctx, txn, codec, id, tree.ObjectLookupFlags{})
-				}
-				name, typeDesc, err := typeLookup(ctx, sqlbase.GetTypeDescID(typ))
-				if err != nil {
-					return err
-				}
-				return typeDesc.HydrateTypeInfoWithName(ctx, typ, &name, sqlbase.TypeLookupFunc(typeLookup))
+			// Given that we never delete members from enum types, a descriptor we
+			// get from the lease manager will be able to be used to decode these stats,
+			// even if it wasn't the descriptor that was used to collect the stats.
+			// If have types that are not backwards compatible in this way, then we
+			// will need to start writing a timestamp on the stats objects and request
+			// TypeDescriptor's with the timestamp that the stats were recorded with.
+			err := sc.ClientDB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+				collection := descs.NewCollection(ctx, sc.Settings, sc.LeaseMgr)
+				defer collection.ReleaseAll(ctx)
+				resolver := descs.NewDistSQLTypeResolver(collection, txn)
+				var err error
+				res.HistogramData.ColumnType, err = resolver.ResolveTypeByOID(ctx, typ.Oid())
+				return err
 			})
 			if err != nil {
 				return nil, err
@@ -431,7 +437,7 @@ func parseStats(
 		var a sqlbase.DatumAlloc
 		for i := range res.Histogram {
 			bucket := &res.HistogramData.Buckets[i]
-			datum, _, err := sqlbase.DecodeTableKey(&a, typ, bucket.UpperBound, encoding.Ascending)
+			datum, _, err := sqlbase.DecodeTableKey(&a, res.HistogramData.ColumnType, bucket.UpperBound, encoding.Ascending)
 			if err != nil {
 				return nil, err
 			}
@@ -476,7 +482,7 @@ ORDER BY "createdAt" DESC
 
 	var statsList []*TableStatistic
 	for _, row := range rows {
-		stats, err := parseStats(ctx, sc.ClientDB, sc.Codec, row)
+		stats, err := sc.parseStats(ctx, row)
 		if err != nil {
 			return nil, err
 		}

pkg/sql/stats/stats_cache_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/lease"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlutil"
 	"github.com/cockroachdb/cockroach/pkg/sql/types"
@@ -233,6 +234,8 @@ func TestCacheBasic(t *testing.T) {
 		db,
 		ex,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	for _, tableID := range tableIDs {
 		if err := checkStatsForTable(ctx, sc, expectedStats[tableID], tableID); err != nil {
@@ -325,6 +328,8 @@ CREATE STATISTICS s FROM tt;
 		kvDB,
 		s.InternalExecutor().(sqlutil.InternalExecutor),
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	tbl := catalogkv.TestingGetTableDescriptor(kvDB, keys.SystemSQLCodec, "t", "tt")
 	// Get stats for our table. We are ensuring here that the access to the stats
@@ -364,6 +369,8 @@ func TestCacheWait(t *testing.T) {
 		db,
 		ex,
 		keys.SystemSQLCodec,
+		s.LeaseManager().(*lease.Manager),
+		s.ClusterSettings(),
 	)
 	for _, tableID := range tableIDs {
 		if err := checkStatsForTable(ctx, sc, expectedStats[tableID], tableID); err != nil {