colfetcher: fix the bytes read statistic collection#75175
colfetcher: fix the bytes read statistic collection#75175craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
5276902 to
81893f3
Compare
rharding6373
left a comment
There was a problem hiding this comment.
Good find. Just a few comments.
Reviewed 1 of 6 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rytaft and @yuzefovich)
-- commits, line 24 at r1:
The issue says that this might be on 21.1. If it is, should we backport it to that release, too? The first sentence in the commit message makes it sound like this was introduced in 21.2, just double checking that's the case.
pkg/sql/colfetcher/cfetcher.go, line 259 at r1 (raw file):
// fetcher is the underlying fetcher that provides KVs. fetcher *row.KVFetcher // bytesRead stores the number of bytes read by fetcher after cFetcher is
Why only store the bytes read after Close and not store the cumulative total in cFetcher?
pkg/sql/colfetcher/index_join.go, line 247 at r1 (raw file):
// still has the references to it. s.spanAssembler.AccountForSpans() s.mu.Lock()
Why not call GetBytesRead() here instead?
pkg/sql/row/kv_fetcher.go, line 141 at r1 (raw file):
return 0 } bytesRead := atomic.LoadInt64(&f.atomics.bytesRead)
Use atomic.SwapInt64 here instead of a separate load and store.
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`). 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.
81893f3 to
174a0d6
Compare
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, rharding6373 (Rachael Harding) wrote…
The issue says that this might be on 21.1. If it is, should we backport it to that release, too? The first sentence in the commit message makes it sound like this was introduced in 21.2, just double checking that's the case.
I checked and 21.1 doesn't have the problem (forgot to update the issue). Adjusted the release note as well.
pkg/sql/colfetcher/cfetcher.go, line 259 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Why only store the bytes read after
Closeand not store the cumulative total incFetcher?
Good point, done.
pkg/sql/colfetcher/index_join.go, line 247 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Why not call
GetBytesRead()here instead?
Removed to due the refactor.
pkg/sql/row/kv_fetcher.go, line 141 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Use
atomic.SwapInt64here instead of a separate load and store.
Nice, thanks! I thought that there must be a method for it but didn't find it when I was writing this code.
rharding6373
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I checked and 21.1 doesn't have the problem (forgot to update the issue). Adjusted the release note as well.
Sounds good!
|
TFTR! bors r+ |
|
Build succeeded: |
During 21.2 release we adjusted the
cFetcherto beClosed 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
cFetcherstarted 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.KVFetcherwas responsible for providing it, and we werezeroing it out. This commit fixes this problem by the
cFetchermemorizing 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
cFetcherwhen 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).Fixes: #75128.
Release note (bug fix): Previously, CockroachDB could incorrectly report
KV bytes readstatistic inEXPLAIN ANALYZEoutput. The bug ispresent only in 21.2.x versions.