colfetcher: track KV CPU time in the direct columnar scan #98264
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
pkg/sql/colfetcher/colbatch_direct_scan.go line 156 at r1 (raw file):
func (s *ColBatchDirectScan) GetKVCPUTime() time.Duration { s.mu.Lock() defer s.mu.Unlock()
[nit] timeutil.CPUStopWatch is already mutex-protected.
This commit addresses a minor TODO to track the KV CPU time when direct columnar scans are used. In the regular columnar scan this time is tracked by the cFetcher, but with the KV projection pushdown the cFetcher is used on the KV server side, so we need to augment the ColBatchDirectScan to track it. Notably, this means that the decoding done on the KV server side is included. Additionally, this commit clarifies how the KV CPU time is obtained from the cFetcher (we don't need to use a helper (unlike in the case of `bytesRead` and `batchRequestsIssued` fields which are written to on `cFetcher.Close`), and we don't need the mutex protection there either). Release note: None
yuzefovich
force-pushed
the
kv-pushdown-cpu
branch
from
5be4aa3 to
e956157
Compare
There was a problem hiding this comment.
No worries, TFTR!
bors r+
Reviewable status:
pkg/sql/colfetcher/colbatch_direct_scan.go line 156 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
[nit]
timeutil.CPUStopWatchis already mutex-protected.
Done. I applied the same change to ColBatchScan and ColIndexJoin too.
|
Build succeeded: |
This commit addresses a minor TODO to track the KV CPU time when direct
columnar scans are used. In the regular columnar scan this time is
tracked by the cFetcher, but with the KV projection pushdown the
cFetcher is used on the KV server side, so we need to augment the
ColBatchDirectScan to track it. Notably, this means that the decoding
done on the KV server side is included. Additionally, this commit
clarifies how the KV CPU time is obtained from the cFetcher (we don't
need to use a helper (unlike in the case of
bytesReadandbatchRequestsIssuedfields which are written to oncFetcher.Close),and we don't need the mutex protection there either).
Epic: CRDB-14837
Release note: None