stats: use leased types in the stats cache

This commit switches the stats cache to use leased types rather than
accessing types directly. This was the final user of the direct type
access methods, so that code is removed as well.

Release note: None

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 {