release-21.1: colfetcher: use the limit hint to size the batch #63093
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We recently merged a couple of commits to take advantage of the optimizer-driven estimated row count that will be output by the cfetcher and of the limit hint when available. However, one scenario was overlooked - if there is no estimate but the limit hint is available, we don't use the latter to size the original batch, and now we do. As an additional optimization this commit refactors the initial state of the state machine so that we didn't allocate the output batch in the constructor of the ColBatchScan but do it only on the very first call to `NextBatch`. This was prompted by the fact that the limit hint is currently only set in `StartScan` which is called in `ColBatchScan.Init`, so without some changes, in the case when the limit hint can be used to size the batch we would allocate a redundant batch of size one in `NewColBatchScan`. I also think that the new initial state of the state machine makes a bit more sense to me. Additionally, this commit generalizes the logic to keep track of the limit hint as "remaining rows to be fetched." Release note: None (this is a follow-up to a recently merged commit that had a release note)
|
This change is |
|
I think this should go in for 21.1.0 because the slowdown could present itself when users are testing new release (before the stats are collected) before switching to it. |
rytaft
approved these changes
Apr 5, 2021
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 1/1 commits from #63020.
/cc @cockroachdb/release
We recently merged a couple of commits to take advantage of the
optimizer-driven estimated row count that will be output by the cfetcher
and of the limit hint when available. However, one scenario was
overlooked - if there is no estimate but the limit hint is available, we
don't use the latter to size the original batch, and now we do.
As an additional optimization this commit refactors the initial state of
the state machine so that we didn't allocate the output batch in the
constructor of the ColBatchScan but do it only on the very first call
to
NextBatch. This was prompted by the fact that the limit hint iscurrently only set in
StartScanwhich is called inColBatchScan.Init, so without some changes, in the case when the limithint can be used to size the batch we would allocate a redundant batch
of size one in
NewColBatchScan. I also think that the new initialstate of the state machine makes a bit more sense to me.
Additionally, this commit generalizes the logic to keep track of the
limit hint as "remaining rows to be fetched."
Addresses: #62212.
Release note: None (this is a follow-up to a recently merged commit that
had a release note)