-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release-21.2: colfetcher: fix the bytes read statistic collection #75260
Conversation
2ca4bdc
to
abfe192
Compare
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
abfe192
to
e93eb09
Compare
I also squashed 562d860 into this backport. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it looks like the new test is failing CI.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @rytaft and @yuzefovich)
During 21.2 release we adjusted the `cFetcher` to be `Close`d eagerly when it is returning the zero-length batch. This was done in order to release some references in order for the memory to be GCed sooner; additionally, the `cFetcher` started being used for the index join where the fetcher is restarted from scratch for every batch of spans, so it seemed reasonable to close it automatically. However, that eager closure broke "bytes read" statistic collection since the `row.KVFetcher` was responsible for providing it, and we were zeroing it out. This commit fixes this problem by the `cFetcher` memorizing the number of bytes it has read in `Close`. Some care needs to be taken to not double-count the bytes read in the index join, so a couple of helper methods have been introduced. Additionally this commit applies the same eager-close optimization to the `cFetcher` when the last batch is returned (which makes it so that if we've just exhausted all KVs, we close the fetcher - previously, we would set the zero length on the batch and might never get into `stateFinished`). This commit additionally includes 562d860 which fixes the bytes read statistic collection for Get requests. Release note (bug fix): Previously, CockroachDB could incorrectly report `KV bytes read` statistic in `EXPLAIN ANALYZE` output. The bug is present only in 21.2.x versions.
e93eb09
to
912f000
Compare
Yep, there were other minor changes on master that this backport needed. |
Backport 1/1 commits from #75175 on behalf of @yuzefovich.
/cc @cockroachdb/release
During 21.2 release we adjusted the
cFetcher
to beClose
d eagerlywhen it is returning the zero-length batch. This was done in order to
release some references in order for the memory to be GCed sooner;
additionally, the
cFetcher
started being used for the index join wherethe fetcher is restarted from scratch for every batch of spans, so it
seemed reasonable to close it automatically.
However, that eager closure broke "bytes read" statistic collection
since the
row.KVFetcher
was responsible for providing it, and we werezeroing it out. This commit fixes this problem by the
cFetcher
memorizing the number of bytes it has read in
Close
. Some care needsto be taken to not double-count the bytes read in the index join, so
a couple of helper methods have been introduced.
Additionally this commit applies the same eager-close optimization to
the
cFetcher
when the last batch is returned (which makes it so thatif we've just exhausted all KVs, we close the fetcher - previously, we
would set the zero length on the batch and might never get into
stateFinished
).This commit additionally includes
562d860 which fixes the bytes read
statistic collection for Get requests.
Fixes: #75128.
Release note (bug fix): Previously, CockroachDB could incorrectly report
KV bytes read
statistic inEXPLAIN ANALYZE
output. The bug ispresent only in 21.2.x versions.
Release justification: low-risk bug fix.