row: set TargetBytes for kvfetcher#45323
Conversation
|
This change is |
|
I think the query type that's most sensitive to fiddling with the request batch sizes is something like COUNT on a large table. This minimizes CPU and maximizes data transfer. |
|
SG. Can you give me a large table (tpcc.x?) that you find representative (enough), just so that I have something concrete to work with? |
|
|
I just tried this via Looking at this again though, for this query I would like the 10k rows limit to be the one taking effect, not the 1mb limit. 1mb ~ 10k rows at an average row size of 100b, which is too small. So I'm just going to bump the limit up to 10mb - I'm mostly trying not to get in the way of "reasonable" scans, all I want to do is curb pathological ones. |
nvb
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde and @tbg)
pkg/server/server_test.go, line 469 at r1 (raw file):
// Iterate through MaxSpanRequestKeys=1..n and TargetBytes=1..m // and (where n and m are chosen to reveal the full result set // in one page). At each(*) combination, paginate
nit: the comment wrapping here is strange
pkg/storage/replica_evaluate.go, line 339 at r1 (raw file):
baHeader.TargetBytes -= retBytes } else { baHeader.TargetBytes = -1
Could you add a comment like we have above for this?
847b2e6 to
8080741
Compare
|
bors r=nvanbenschoten |
This PR finishes up the work initiated in cockroachdb#44925 to allow (forward and reverse) scans to specify a TargetBytes hint which (mod overshooting by one row) restricts the size of the responses. The plan is to use it in kvfetcher, however this is left as a separate commit - here we focus on testing the functionality only. Release note: None
Fixes cockroachdb#33660. Release note (bug fix): Significantly reduce the amount of memory allocated while scanning tables with a large average row size.
We had lowered this because the lack of kvfetcher-level result size limiting caused OOMs (the lower limit still caused OOMs, though more rarely). Now kvfetcher does bound the size of its result sets, so this should just work. Release note: None
660b3e7 bumped the default range size by a factor of 8. Do the same in this test. Addresses one failure mode of cockroachdb#33660. Release note: None
15a38a5 to
de24cbd
Compare
|
bors r=nvanbenschoten |
Build succeeded |
This finishes up the work in #44925 and completes the TargetBytes functionality. This is then used in kvfetcher, which henceforth aims to return no more than ~1mb per request. Additional commits restore the hotspotsplits roachtest and fix it. Reverting the relevant commit from this PR, the test failed nine out of ten times). With all commits, it passed ten times.
The one question I have whether TargetBytes should be set to a value higher than 1mb to avoid a performance regression (where should I look?).
Fixes #33660.