backupccl: do not backup or restore stat forecasts

[stevendanna]

Statistics forecasts are not persisted and are reconstructed on demand from the persisted statistics. As such, they don't need to be included in a backup or restored.

This has the additional effect of solving a bug in which we would fail to write statistics into our metadata SST if it included more than 1 forecast since the in-memory forecasts have a 0 statistic_id.

Fixes #86806

Epic: none

Release note: None

[cockroach-teamcity]

This change is 

[michae2]

Thank you for fixing this! Hope this wasn't too much of an inconvenience!

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @benbardin and @stevendanna)

---

pkg/ccl/backupccl/backup_job.go line 376 at r1 (raw file):

				// from the persisted stats and do not
				// need to be backed up.
				if stat.Name == jobspb.ForecastStatsName {

Additionally, in v23.1 we also want to skip merged partial statistics, which can be detected with stat.Name == jobspb.MergedStatsName.

---

pkg/ccl/backupccl/restore_job.go line 532 at r1 (raw file):

	tableHasStatsInBackup := make(map[descpb.ID]struct{})
	for _, stat := range tableStatistics {
		if stat.Name == jobspb.ForecastStatsName {

We should also skip merged partial statistics here (stat.Name == jobspb.MergedStatsName).

---

pkg/ccl/backupccl/testdata/backup-restore/metadata line 34 at r1 (raw file):

# The previous bug occurs when there are two or more forecasts since
# forecasts do not hava a statistics ID.

Note that whenever #86358 is fixed, forecasted stats will have a statistics ID. (And same for merged partial stats, eventually.)

[stevendanna]

@michae2 Good call on the merged stats as those also pose a problem. Fixing that up actually exposed another potential problem. It seems like we aren't backing up partial statistics because they aren't returned from (*TableStatisticsCache).GetTableStats.

How would you feel about a new method on the stats cache that was something like GetTableStatsForBackup?

[stevendanna]

Restoring partial statistics might require us to complicate the code for restoring statistics a bit since partial statistics reference full statistics and those references would need to be fixed up on restore.

This seems like something we could do in a follow-up issue.

[stevendanna]

I opened #95056 to track the other issue.

[michae2]

@michae2 Good call on the merged stats as those also pose a problem. Fixing that up actually exposed another potential problem. It seems like we aren't backing up partial statistics because they aren't returned from (*TableStatisticsCache).GetTableStats.

How would you feel about a new method on the stats cache that was something like GetTableStatsForBackup?

I opened #95056 to track the other issue.

Ah, good catch. I think we will eventually want the optimizer to see partial stats via GetTableStats anyway, so let me quickly fix that.

Restoring partial statistics might require us to complicate the code for restoring statistics a bit since partial statistics reference full statistics and those references would need to be fixed up on restore.

This seems like something we could do in a follow-up issue.

Yes, this is a problem. Also a problem for ALTER TABLE ... INJECT STATISTICS. One idea is to change InsertNewStat to also insert the StatisticID. I can open an issue about this.

[michae2]

Thank you!

Reviewed 3 of 3 files at r2, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @benbardin and @stevendanna)

---

pkg/ccl/backupccl/backup_job.go line 426 at r4 (raw file):

	// stats on demand and do not need to be backed up or
	// restored.
	if stat.Name == jobspb.ForecastStatsName || stat.Name == jobspb.MergedStatsName {

nit: This could be simplified to return stat.Name != jobspb.ForecastStatsName && stat.Name != jobspb.MergedStatsName.

[stevendanna]

bors r=michae2

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)