sql/stats: include partial stats in results of statsCache.GetTableStats

[michae2]

We were not including partial stats in the list of table statistics returned by statsCache.GetTableStats. This was fine for the optimizer, which currently cannot use partial stats directly, but it was a problem for backup.

We'd like to use partial stats directly in the optimizer eventually, so this commit goes ahead and adds them to the results of GetTableStats. The optimizer then must filter them out. To streamline this we add some helper functions.

Finally, in an act of overzealous refactoring, this commit also changes MergedStatistics and ForecastTableStatistics to accept partial statistics and full statistics mixed together in the same input list. This simplifies the code that calls these functions.

Fixes: #95056
Part of: #93983

Epic: CRDB-19449

Release note: None

[cockroach-teamcity]

This change is 

[stevendanna]

Code looks pretty reasonable to me. I like the various predicate methods you added.

Should we perhaps filter partial stats from backupRestore with a TODO until we fix the insertion of partial stats? Unless I'm mistaken (very possible), I think we probably do the wrong thing with the FullHistogramID on the partial stat.

[michae2]

Should we perhaps filter partial stats from backupRestore with a TODO until we fix the insertion of partial stats? Unless I'm mistaken (very possible), I think we probably do the wrong thing with the FullHistogramID on the partial stat.

Ah, thanks for reminding me, I forgot to file an issue!

I think we should go ahead and backup and restore them anyway. It won't break anything serious if we do the wrong thing with the FullHistogramID, it just means we won't be able to create those merged stats after restore.

[michae2]

Should we perhaps filter partial stats from backupRestore with a TODO until we fix the insertion of partial stats? Unless I'm mistaken (very possible), I think we probably do the wrong thing with the FullHistogramID on the partial stat.

Ah, thanks for reminding me, I forgot to file an issue!

Updated #94101 about this.

[rytaft]

nice cleanup!

Reviewed 10 of 14 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @michae2)

---

pkg/sql/distsql_plan_stats.go line 230 at r2 (raw file):

		// someone could create a statistic named __forecast__ or __merged__.
		// Update system.table_statistics to add an enum to indicate which
		// type of statistic it is.

nit: update comment now that you've removed the name references

---

pkg/sql/stats/forecast.go line 67 at r2 (raw file):

// return the same forecasts.
func ForecastTableStatistics(ctx context.Context, observed []*TableStatistic) []*TableStatistic {
	// Early sanity check. We'll check this again in forecastColumnStatistics.

Is this not needed anymore?

[michae2]

Thank you!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @rytaft)

---

pkg/sql/distsql_plan_stats.go line 230 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

nit: update comment now that you've removed the name references

I ended up removing it altogether in favor of a comment on #50655.

---

pkg/sql/stats/forecast.go line 67 at r2 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Is this not needed anymore?

Yeah, it was only really needed to protect the latest := observed[0].CreatedAt which has become part of the first loop over observed. Now if len(observed) is zero then latest.IsZero() will be true after the first loop, and we will return nil. If len(observed) is less than minObservationsForForecast we will catch it at the beginning of forecastColumnStatistics.

[rytaft]

Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner)

[michae2]

bors r=rytaft

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)