colfetcher: don't use the cFetcher's allocator for other stuff#94674
colfetcher: don't use the cFetcher's allocator for other stuff#94674craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
This commit fixes a minor bug in how we were accounting for the copy of the spans in the ColBatchScan. In particular, the `cFetcher` (or, more precisely, the `colmem.SetAccountingHelper`) requires that the allocator is not shared with other components (so that we could quickly get the footprint of the batch). This was violated in a single spot and is now fixed. The impact of this oversight is very minor - the set accounting helper would overestimate the footprint of the batch, so it could be returning batches smaller than the limit. Thus, the fix doesn't need to be backported. I also audited other users of the set accounting helper and didn't find any other violations. Epic: None Release note: None
|
This change is |
DrewKimball
left a comment
There was a problem hiding this comment.
Maybe we should have operators that can't share an allocator construct it themselves. Not necessarily a suggestion for this PR, but wanted to get your thoughts on the idea.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @michae2)
|
It's a good question. The main pain point would be to provide a way to construct a memory account. My initial thought was that we could pass a reference to TFTR! bors r+ |
|
Build succeeded: |
This commit fixes a minor bug in how we were accounting for the copy of the spans in the ColBatchScan. In particular, the
cFetcher(or, more precisely, thecolmem.SetAccountingHelper) requires that the allocator is not shared with other components (so that we could quickly get the footprint of the batch). This was violated in a single spot and is now fixed. The impact of this oversight is very minor - the set accounting helper would overestimate the footprint of the batch, so it could be returning batches smaller than the limit. Thus, the fix doesn't need to be backported.I also audited other users of the set accounting helper and didn't find any other violations.
Epic: None
Release note: None