opt: fix issue with dangling pointer in statisticsBuilder #37983
Conversation
|
This change is |
There was a problem hiding this comment.
Are you worried that we'll continue having this kind of bug in the future, given the subtlety of it? I wonder if we should change ColStatsMap and colStatVal to directly reference *ColumnStatistic rather than keeping a pos into the initial array and other slice. This would require us to allocate ColumnStatistic objects on the heap once we got past the initial 3, but that's an uncommon case, so probably wouldn't have measurable impact on perf.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @justinj and @RaduBerinde)
|
This is definitely a possibility. My intuition is that I suppose one solution would be to just do this simple fix of re-fetching the column stat now but keep an eye out for this bug in the near future and implement your suggested change if it looks like the bug is rearing it's head again. What do you think? |
There was a problem hiding this comment.
Friendly ping
Reviewable status:
There was a problem hiding this comment.
That sounds OK. Though maybe add additional comments in ColStatsMap mutable methods to warn callers to be careful.
Otherwise,
Reviewable status:
This commit fixes an issue where the statisticsBuilder was using an invalid pointer to update statistics for some multi-column stats in the local table statistics cache. The fix is to re-fetch the pointer to the stats just before updating them, in case the previously fetched address is no longer valid. This problem is very hard to reproduce, so I had to use the somewhat complex query from the issue for a regression test. Fixes cockroachdb#37953 Release note: None
|
TFTR! Added comments above all functions in |
|
bors r+ |
37983: opt: fix issue with dangling pointer in statisticsBuilder r=rytaft a=rytaft This commit fixes an issue where the `statisticsBuilder` was using an invalid pointer to update statistics for some multi-column stats in the local table statistics cache. The fix is to re-fetch the pointer to the stats just before updating them, in case the previously fetched address is no longer valid. This problem is very hard to reproduce, so I had to use the somewhat complex query from the issue for a regression test. Fixes #37953 Release note: None Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Build succeeded |
This commit fixes an issue where the
statisticsBuilderwas usingan invalid pointer to update statistics for some multi-column stats
in the local table statistics cache. The fix is to re-fetch the
pointer to the stats just before updating them, in case the
previously fetched address is no longer valid.
This problem is very hard to reproduce, so I had to use the somewhat
complex query from the issue for a regression test.
Fixes #37953
Release note: None