backupccl: do not backup or restore stat forecasts#94992
backupccl: do not backup or restore stat forecasts#94992craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
55d091a to
f8fab63
Compare
f8fab63 to
5d457d9
Compare
michae2
left a comment
There was a problem hiding this comment.
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.)
5d457d9 to
59e3627
Compare
|
@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 How would you feel about a new method on the stats cache that was something like GetTableStatsForBackup? |
|
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. |
2014eb7 to
68b3f84
Compare
|
I opened #95056 to track the other issue. |
Ah, good catch. I think we will eventually want the optimizer to see partial stats via
Yes, this is a problem. Also a problem for |
michae2
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:
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.
68b3f84 to
9b89a3b
Compare
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 cockroachdb#86806 Epic: none Release note: None
Merged statistics are generated from partial statistics and the related full statistic. Partial statistics are not currently included in the backup, but will be soon, thus we are excluding merged statistics from the backup. Release note: None
9b89a3b to
c8060fd
Compare
|
bors r=michae2 |
|
Build succeeded: |
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