stats: use table descriptors instead of IDs

Previously, the sql stats package inferred various properties about a table from its ID, like if it is a system or a virtual table. However the table descriptor is usually readily or easily available, providing much richer information about a table than its ID. In particular, this allows us to stop collecting stats for views. Release note (sql change): table statistics are no longer collected for views.
cockroachdb · Aug 17, 2021 · 7273596 · 7273596
1 parent 7018384
commit 7273596
Show file tree

Hide file tree

Showing 26 changed files with 148 additions and 107 deletions.
diff --git a/pkg/ccl/backupccl/backup_job.go b/pkg/ccl/backupccl/backup_job.go
@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/covering"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -343,9 +344,10 @@ func backup(
 	}
 	var tableStatistics []*stats.TableStatisticProto
 	for i := range backupManifest.Descriptors {
-		if tableDesc, _, _, _ := descpb.FromDescriptor(&backupManifest.Descriptors[i]); tableDesc != nil {
+		if tbl, _, _, _ := descpb.FromDescriptor(&backupManifest.Descriptors[i]); tbl != nil {
+			tableDesc := tabledesc.NewBuilder(tbl).BuildImmutableTable()
 			// Collect all the table stats for this table.
-			tableStatisticsAcc, err := statsCache.GetTableStats(ctx, tableDesc.GetID())
+			tableStatisticsAcc, err := statsCache.GetTableStats(ctx, tableDesc)
 			if err != nil {
 				// Successfully backed up data is more valuable than table stats that can
 				// be recomputed after restore, and so if we fail to collect the stats of a

diff --git a/pkg/ccl/backupccl/restore_job.go b/pkg/ccl/backupccl/restore_job.go
@@ -1830,7 +1830,8 @@ func (r *restoreResumer) ReportResults(ctx context.Context, resultsCh chan<- tre
 func (r *restoreResumer) notifyStatsRefresherOfNewTables() {
 	details := r.job.Details().(jobspb.RestoreDetails)
 	for i := range details.TableDescs {
-		r.execCfg.StatsRefresher.NotifyMutation(details.TableDescs[i].GetID(), math.MaxInt32 /* rowsAffected */)
+		desc := tabledesc.NewBuilder(details.TableDescs[i]).BuildImmutableTable()
+		r.execCfg.StatsRefresher.NotifyMutation(desc, math.MaxInt32 /* rowsAffected */)
 	}
 }
 

diff --git a/pkg/ccl/importccl/import_stmt.go b/pkg/ccl/importccl/import_stmt.go
@@ -2273,7 +2273,8 @@ func (r *importResumer) publishTables(ctx context.Context, execCfg *sql.Executor
 	// rows affected per table, so we use a large number because we want to make
 	// sure that stats always get created/refreshed here.
 	for i := range details.Tables {
-		execCfg.StatsRefresher.NotifyMutation(details.Tables[i].Desc.ID, math.MaxInt32 /* rowsAffected */)
+		desc := tabledesc.NewBuilder(details.Tables[i].Desc).BuildImmutableTable()
+		execCfg.StatsRefresher.NotifyMutation(desc, math.MaxInt32 /* rowsAffected */)
 	}
 
 	return nil

diff --git a/pkg/sql/authorization.go b/pkg/sql/authorization.go
@@ -162,7 +162,7 @@ func getOwnerOfDesc(desc catalog.Descriptor) security.SQLUsername {
 	if owner.Undefined() {
 		// If the descriptor is ownerless and the descriptor is part of the system db,
 		// node is the owner.
-		if desc.GetID() == keys.SystemDatabaseID || desc.GetParentID() == keys.SystemDatabaseID {
+		if catalog.IsSystemDescriptor(desc) {
 			owner = security.NodeUserName()
 		} else {
 			// This check is redundant in this case since admin already has privilege

diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go
@@ -477,3 +477,12 @@ func FormatSafeDescriptorProperties(w *redact.StringBuilder, desc Descriptor) {
 		w.Printf(", NumDrainingNames: %d", len(drainingNames))
 	}
 }
+
+// IsSystemDescriptor returns true iff the descriptor is a system or a reserved
+// descriptor.
+func IsSystemDescriptor(desc Descriptor) bool {
+	if desc.GetID() <= keys.MaxReservedDescID {
+		return true
+	}
+	return desc.GetParentID() == keys.SystemDatabaseID
+}
diff --git a/pkg/sql/catalog/lease/helpers_test.go b/pkg/sql/catalog/lease/helpers_test.go
@@ -161,7 +161,7 @@ func (m *Manager) PublishMultiple(
 			if err != nil {
 				return nil, err
 			}
-			expectedVersions[id] = expected
+			expectedVersions[id] = expected.GetVersion()
 		}
 
 		descs := make(map[descpb.ID]catalog.MutableDescriptor)

diff --git a/pkg/sql/catalog/lease/lease.go b/pkg/sql/catalog/lease/lease.go
@@ -106,32 +106,30 @@ func (m *Manager) WaitForNoVersion(
 }
 
 // WaitForOneVersion returns once there are no unexpired leases on the
-// previous version of the descriptor. It returns the current version.
+// previous version of the descriptor. It returns the descriptor with the
+// current version.
 // After returning there can only be versions of the descriptor >= to the
 // returned version. Lease acquisition (see acquire()) maintains the
 // invariant that no new leases for desc.Version-1 will be granted once
 // desc.Version exists.
 func (m *Manager) WaitForOneVersion(
 	ctx context.Context, id descpb.ID, retryOpts retry.Options,
-) (descpb.DescriptorVersion, error) {
-	var version descpb.DescriptorVersion
+) (desc catalog.Descriptor, _ error) {
 	for lastCount, r := 0, retry.Start(retryOpts); r.Next(); {
-		var desc catalog.Descriptor
 		if err := m.DB().Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
 			desc, err = catalogkv.MustGetDescriptorByID(ctx, txn, m.Codec(), id)
 			return err
 		}); err != nil {
-			return 0, err
+			return nil, err
 		}
 
 		// Check to see if there are any leases that still exist on the previous
 		// version of the descriptor.
 		now := m.storage.clock.Now()
 		descs := []IDVersion{NewIDVersionPrev(desc.GetName(), desc.GetID(), desc.GetVersion())}
-		version = desc.GetVersion()
 		count, err := CountLeases(ctx, m.storage.internalExecutor, descs, now)
 		if err != nil {
-			return 0, err
+			return nil, err
 		}
 		if count == 0 {
 			break
@@ -141,7 +139,7 @@ func (m *Manager) WaitForOneVersion(
 			log.Infof(ctx, "waiting for %d leases to expire: desc=%v", count, descs)
 		}
 	}
-	return version, nil
+	return desc, nil
 }
 
 // IDVersion represents a descriptor ID, version pair that are

diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go
@@ -2746,8 +2746,7 @@ func (ex *connExecutor) notifyStatsRefresherOfNewTables(ctx context.Context) {
 			// Initiate a run of CREATE STATISTICS. We use a large number
 			// for rowsAffected because we want to make sure that stats always get
 			// created/refreshed here.
-			ex.planner.execCfg.StatsRefresher.
-				NotifyMutation(desc.GetID(), math.MaxInt32 /* rowsAffected */)
+			ex.planner.execCfg.StatsRefresher.NotifyMutation(desc, math.MaxInt32 /* rowsAffected */)
 		}
 	}
 }

diff --git a/pkg/sql/delete.go b/pkg/sql/delete.go
@@ -140,10 +140,7 @@ func (d *deleteNode) BatchedNext(params runParams) (bool, error) {
 	}
 
 	// Possibly initiate a run of CREATE STATISTICS.
-	params.ExecCfg().StatsRefresher.NotifyMutation(
-		d.run.td.tableDesc().GetID(),
-		d.run.td.lastBatchSize,
-	)
+	params.ExecCfg().StatsRefresher.NotifyMutation(d.run.td.tableDesc(), d.run.td.lastBatchSize)
 
 	return d.run.td.lastBatchSize > 0, nil
 }

diff --git a/pkg/sql/delete_range.go b/pkg/sql/delete_range.go
@@ -170,7 +170,7 @@ func (d *deleteRangeNode) startExec(params runParams) error {
 	}
 
 	// Possibly initiate a run of CREATE STATISTICS.
-	params.ExecCfg().StatsRefresher.NotifyMutation(d.desc.GetID(), d.rowCount)
+	params.ExecCfg().StatsRefresher.NotifyMutation(d.desc, d.rowCount)
 
 	return nil
 }

diff --git a/pkg/sql/distsql_plan_stats.go b/pkg/sql/distsql_plan_stats.go
@@ -200,7 +200,7 @@ func (dsp *DistSQLPlanner) createStatsPlan(
 	)
 
 	// Estimate the expected number of rows based on existing stats in the cache.
-	tableStats, err := planCtx.ExtendedEvalCtx.ExecCfg.TableStatsCache.GetTableStats(planCtx.ctx, desc.GetID())
+	tableStats, err := planCtx.ExtendedEvalCtx.ExecCfg.TableStatsCache.GetTableStats(planCtx.ctx, desc)
 	if err != nil {
 		return nil, err
 	}

diff --git a/pkg/sql/gcjob/gc_job.go b/pkg/sql/gcjob/gc_job.go
@@ -104,7 +104,8 @@ func (r schemaChangeGCResumer) Resume(ctx context.Context, execCtx interface{})
 	if len(details.InterleavedIndexes) > 0 {
 		// Before deleting any indexes, ensure that old versions of the table
 		// descriptor are no longer in use.
-		if err := sql.WaitToUpdateLeases(ctx, execCfg.LeaseManager, details.InterleavedTable.ID); err != nil {
+		_, err := sql.WaitToUpdateLeases(ctx, execCfg.LeaseManager, details.InterleavedTable.ID)
+		if err != nil {
 			return err
 		}
 		interleavedIndexIDs := make([]descpb.IndexID, len(details.InterleavedIndexes))

diff --git a/pkg/sql/gcjob/index_garbage_collection.go b/pkg/sql/gcjob/index_garbage_collection.go
@@ -19,7 +19,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -44,18 +43,15 @@ func gcIndexes(
 	// Before deleting any indexes, ensure that old versions of the table descriptor
 	// are no longer in use. This is necessary in the case of truncate, where we
 	// schedule a GC Job in the transaction that commits the truncation.
-	if err := sql.WaitToUpdateLeases(ctx, execCfg.LeaseManager, parentID); err != nil {
+	parentDesc, err := sql.WaitToUpdateLeases(ctx, execCfg.LeaseManager, parentID)
+	if err != nil {
 		return err
 	}
 
-	var parentTable catalog.TableDescriptor
-	if err := execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
-		parentTable, err = catalogkv.MustGetTableDescByID(ctx, txn, execCfg.Codec, parentID)
-		return err
-	}); err != nil {
-		return errors.Wrapf(err, "fetching parent table %d", parentID)
+	parentTable, isTable := parentDesc.(catalog.TableDescriptor)
+	if !isTable {
+		return errors.AssertionFailedf("expected descriptor %d to be a table, not %T", parentID, parentDesc)
 	}
-
 	for _, index := range droppedIndexes {
 		if index.Status != jobspb.SchemaChangeGCProgress_DELETING {
 			continue

diff --git a/pkg/sql/insert.go b/pkg/sql/insert.go
@@ -262,7 +262,7 @@ func (n *insertNode) BatchedNext(params runParams) (bool, error) {
 	}
 
 	// Possibly initiate a run of CREATE STATISTICS.
-	params.ExecCfg().StatsRefresher.NotifyMutation(n.run.ti.tableDesc().GetID(), n.run.ti.lastBatchSize)
+	params.ExecCfg().StatsRefresher.NotifyMutation(n.run.ti.tableDesc(), n.run.ti.lastBatchSize)
 
 	return n.run.ti.lastBatchSize > 0, nil
 }

diff --git a/pkg/sql/insert_fast_path.go b/pkg/sql/insert_fast_path.go
@@ -309,7 +309,7 @@ func (n *insertFastPathNode) BatchedNext(params runParams) (bool, error) {
 	n.run.done = true
 
 	// Possibly initiate a run of CREATE STATISTICS.
-	params.ExecCfg().StatsRefresher.NotifyMutation(n.run.ti.ri.Helper.TableDesc.GetID(), len(n.input))
+	params.ExecCfg().StatsRefresher.NotifyMutation(n.run.ti.ri.Helper.TableDesc, len(n.input))
 
 	return true, nil
 }

diff --git a/pkg/sql/opt_catalog.go b/pkg/sql/opt_catalog.go
@@ -419,7 +419,7 @@ func (oc *optCatalog) dataSourceForTable(
 	var tableStats []*stats.TableStatistic
 	if !flags.NoTableStats {
 		var err error
-		tableStats, err = oc.planner.execCfg.TableStatsCache.GetTableStats(context.TODO(), desc.GetID())
+		tableStats, err = oc.planner.execCfg.TableStatsCache.GetTableStats(context.TODO(), desc)
 		if err != nil {
 			// Ignore any error. We still want to be able to run queries even if we lose
 			// access to the statistics table.

diff --git a/pkg/sql/schema_changer.go b/pkg/sql/schema_changer.go
@@ -602,7 +602,8 @@ func (sc *SchemaChanger) exec(ctx context.Context) error {
 	// returns, so that the new schema is live everywhere. This is not needed for
 	// correctness but is done to make the UI experience/tests predictable.
 	waitToUpdateLeases := func(refreshStats bool) error {
-		if err := WaitToUpdateLeases(ctx, sc.leaseMgr, sc.descID); err != nil {
+		latestDesc, err := WaitToUpdateLeases(ctx, sc.leaseMgr, sc.descID)
+		if err != nil {
 			if errors.Is(err, catalog.ErrDescriptorNotFound) {
 				return err
 			}
@@ -614,7 +615,7 @@ func (sc *SchemaChanger) exec(ctx context.Context) error {
 		// We wait to trigger a stats refresh until we know the leases have been
 		// updated.
 		if refreshStats {
-			sc.refreshStats()
+			sc.refreshStats(latestDesc)
 		}
 		return nil
 	}
@@ -761,7 +762,8 @@ func (sc *SchemaChanger) handlePermanentSchemaChangeError(
 	// returns, so that the new schema is live everywhere. This is not needed for
 	// correctness but is done to make the UI experience/tests predictable.
 	waitToUpdateLeases := func(refreshStats bool) error {
-		if err := WaitToUpdateLeases(ctx, sc.leaseMgr, sc.descID); err != nil {
+		desc, err := WaitToUpdateLeases(ctx, sc.leaseMgr, sc.descID)
+		if err != nil {
 			if errors.Is(err, catalog.ErrDescriptorNotFound) {
 				return err
 			}
@@ -773,7 +775,7 @@ func (sc *SchemaChanger) handlePermanentSchemaChangeError(
 		// We wait to trigger a stats refresh until we know the leases have been
 		// updated.
 		if refreshStats {
-			sc.refreshStats()
+			sc.refreshStats(desc)
 		}
 		return nil
 	}
@@ -1009,7 +1011,9 @@ func (sc *SchemaChanger) createIndexGCJob(
 
 // WaitToUpdateLeases until the entire cluster has been updated to the latest
 // version of the descriptor.
-func WaitToUpdateLeases(ctx context.Context, leaseMgr *lease.Manager, descID descpb.ID) error {
+func WaitToUpdateLeases(
+	ctx context.Context, leaseMgr *lease.Manager, descID descpb.ID,
+) (catalog.Descriptor, error) {
 	// Aggressively retry because there might be a user waiting for the
 	// schema change to complete.
 	retryOpts := retry.Options{
@@ -1018,9 +1022,13 @@ func WaitToUpdateLeases(ctx context.Context, leaseMgr *lease.Manager, descID des
 		Multiplier:     1.5,
 	}
 	log.Infof(ctx, "waiting for a single version...")
-	version, err := leaseMgr.WaitForOneVersion(ctx, descID, retryOpts)
+	desc, err := leaseMgr.WaitForOneVersion(ctx, descID, retryOpts)
+	var version descpb.DescriptorVersion
+	if desc != nil {
+		version = desc.GetVersion()
+	}
 	log.Infof(ctx, "waiting for a single version... done (at v %d)", version)
-	return err
+	return desc, err
 }
 
 // done finalizes the mutations (adds new cols/indexes to the table).
@@ -1465,11 +1473,13 @@ func (sc *SchemaChanger) runStateMachineAndBackfill(ctx context.Context) error {
 	return sc.done(ctx)
 }
 
-func (sc *SchemaChanger) refreshStats() {
+func (sc *SchemaChanger) refreshStats(desc catalog.Descriptor) {
 	// Initiate an asynchronous run of CREATE STATISTICS. We use a large number
 	// for rowsAffected because we want to make sure that stats always get
 	// created/refreshed here.
-	sc.execCfg.StatsRefresher.NotifyMutation(sc.descID, math.MaxInt32 /* rowsAffected */)
+	if tableDesc, ok := desc.(catalog.TableDescriptor); ok {
+		sc.execCfg.StatsRefresher.NotifyMutation(tableDesc, math.MaxInt32 /* rowsAffected */)
+	}
 }
 
 // maybeReverseMutations reverses the direction of all the mutations with the

diff --git a/pkg/sql/stats/BUILD.bazel b/pkg/sql/stats/BUILD.bazel
@@ -25,6 +25,7 @@ go_library(
         "//pkg/roachpb:with-mocks",
         "//pkg/settings",
         "//pkg/settings/cluster",
+        "//pkg/sql/catalog",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
         "//pkg/sql/catalog/systemschema",
@@ -82,9 +83,11 @@ go_test(
         "//pkg/server",
         "//pkg/settings/cluster",
         "//pkg/sql",
+        "//pkg/sql/catalog",
         "//pkg/sql/catalog/catalogkv",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/descs",
+        "//pkg/sql/catalog/tabledesc",
         "//pkg/sql/row",
         "//pkg/sql/rowenc",
         "//pkg/sql/rowexec",