sql/stats: collect statistics over system tables

[irfansharif]

Is your feature request related to a problem? Please describe.

We're using system tables for increasingly sophisticated usages, mostly around driving subsystems in CRDB. system.span_configurations is one example and is used to drive the span configs infrastructure. We currently don't auto-collect statistics over system tables, and when statistics are available (if triggered manually through ANALYZE system.some_table for e.g.) we ignore system table statistics. See:

cockroach/pkg/sql/stats/stats_cache.go

Lines 225 to 229 in b01d6c7

	if catalog.IsSystemDescriptor(table) {
	// Don't try to get statistics for system tables (most importantly,
	// for table_statistics itself).
	return false
	}

Describe the solution you'd like

For us to collect stats over system tables (we could either have tables opt-in or opt-out depending; we have some uncertainty around whether it made sense to consult stats over system.table_statistics).

Describe alternatives you've considered

Not doing anything, which has severe performance implications for the span configs infrastructure. In #78215 we attributed the lack of statistics to the reason why the optimizer chooses a full table scan for an internal query, as opposed to bounded segmented index scans. For a large number of schemas, this is going to be slow.

+cc @rytaft, @ajwerner.

Jira issue: CRDB-15838

[mgartner]

There's two parts to this:

1. Unblock @irfansharif by loading system table stats into the stats cache.
2. Auto-collect table stats for system tables.

[msirek]

@irfansharif In this issue you mention system.table_statistics. The PR I opened enables stats collection on any system table except for system.table_statistics and system.lease. I know stats on system.lease is problematic, but just assumed stats on system.table_statistics might be as well. Do you think you'll need to collect stats on system.table_statistics? If so, I might take a closer look.

[irfansharif]

I don't need statistics over system.table_statistics, no; was just reporting that I observed the same hangs you did when I tried a naive form of #80761. Thanks for the PR! Feel free to close this issue out after it lands.

[mgartner]

Is there a reason we should automatically collect stats on system tables? Or is manual collection for system tables sufficient?

[irfansharif]

Some of these system tables are driven by automatic processes/jobs. system.span_configurations for example is being driven by something the reconciler you'll see when running SHOW AUTOMATIC JOBS. I think we want automatic statistics; that table's being maintained with every schema change. In #78215 (comment) we found that without automatic collection, we'll fall back to full table scans for a critical part of the the subsystem. Does #80761 also introduce auto stats collection for these tables?

[mgartner]

Does #80761 also introduce auto stats collection for these tables?

It does not, but that's something we can do in a follow-up PR. Unless @msirek or @rytaft have a reason why we shouldn't enable auto-stats for system tables.

[msirek]

Unless @msirek or @rytaft have a reason why we shouldn't enable auto-stats for system tables.

I don't know of any specific reason not to enable it, but I can imagine if some system tables are small and constantly getting updates, it might trigger a lot of stats collections, and I'm not sure if the extra reads from system tables might cause contention with writes. Also, with stats on system tables changing a lot, what would the stability of query plans for internal queries be like? With no stats, nothing is changing, but with fluctuating stats, plans will change, and if they change for the worse that may be difficult to debug. Regressions in system queries may also affect system stability so could have a greater impact than regressions in user queries. Though maybe our internal queries aren't too complex. I haven't really looked at all of them.

[irfansharif]

but I can imagine if some system tables are small and constantly getting updates, it might trigger a lot of stats collections, and I'm not sure if the extra reads from system tables might cause contention with writes.

Is this concern any different from small + write-heavy user tables? Or is it just that our existing system tables so far haven't functioned with auto stats, so we should tread carefully. That's fair -- at the very least I'd like system.span_configurations to be opted into the stats collection. Perhaps we should ask teams responsible for the other tables to opt-in/out.

[msirek]

Or is it just that our existing system tables so far haven't functioned with auto stats, so we should tread carefully.

Yes, mainly just the fear of adding more moving parts to the system. I don't know if anyone else would be worried about this. Having autostats on just a subset of system tables sounds like a good option, but maybe my fears are overblown. Would need to take a closer look.

[mgartner]

I don't think the reads on system tables will contend with writes because we use AS OF SYSTEM TIME when scanning tables for generating automatic stats:

cockroach/pkg/sql/stats/automatic_stats.go

Lines 493 to 508 in 0fcbb3c

	func (r *Refresher) refreshStats(ctx context.Context, tableID descpb.ID, asOf time.Duration) error {
	// Create statistics for all default column sets on the given table.
	_ /* rows */, err := r.ex.Exec(
	ctx,
	"create-stats",
	nil, /* txn */
	fmt.Sprintf(
	"CREATE STATISTICS %s FROM [%d] WITH OPTIONS THROTTLING %g AS OF SYSTEM TIME '-%s'",
	jobspb.AutoStatsName,
	tableID,
	AutomaticStatisticsMaxIdleTime.Get(&r.st.SV),
	asOf.String(),
	),
	)
	return err
	}

I'm hesitant about introducing this too. We could put it behind a setting that defaults to off. If we got confidence in it before the 22.2 release we could switch it to on by default.

[msirek]

If we got confidence in it before the 22.2 release we could switch it to on by default.

Or we could default it to on at first to get more testing exposure, then switch it off before 22.2 if we see any abnormalities or suspicious slowdowns in tests.

[rytaft]

Sorry for joining the discussion late...

Or we could default it to on at first to get more testing exposure, then switch it off before 22.2 if we see any abnormalities or suspicious slowdowns in tests.

Yes, I'd be in favor of enabling this now, and we can disable it later if need be.