sql: only create a LeafTxn for local flows if Streamer is enabled #76641
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
|
This change is |
yuzefovich
force-pushed
the
streamer-cleanup
branch
2 times, most recently
from
6fe264d to
0fe540b
Compare
There was a problem hiding this comment.
Good work!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @rharding6373 and @yuzefovich)
pkg/sql/distsql_running.go, line 459 at r1 (raw file):
// Even if planCtx.isLocal is false (which is the case when we think it's // worth distributing the query), we need to go through the processors to // figure out whether any of them have concurrency.
nit: a comment here about why this is only relevant if we may be using the streamer would be helpful here.
Previously, during the flow setup stage we would always choose to create a LeafTxn for local flows if there is a processor that might use a Streamer API. This was the case even when the Streamer is disabled by a cluster setting. Such behavior is a regression (since leaf txns don't have a transparent span refresh mechanism and read spans have to be collected with the metadata at the end of the execution), so this commit fixes things by only using a LeafTxn if the Streamer is actually enabled. Now, the users of the Streamer API are expected to check that they do have a LeafTxn and only use the Streamer if so. Additionally, this commit correctly populates `HasConcurrency` field for the local flows even when the query might be distributed. The fix is needed since the physical planning decision to distribute the query or not is finalized after we decide which txn to use for the flow, which is too late. Thus, whenever the Streamer is enabled and we find an index join in the plan, we will make sure to use the LeafTxn (this was already the case if the plan ended up being distributed). Release note: None
yuzefovich
force-pushed
the
streamer-cleanup
branch
from
0fe540b to
7b0ef97
Compare
yuzefovich
commented
Feb 17, 2022
There was a problem hiding this comment.
Extended the comment.
TFTR!
bors r+
Reviewable status:
|
Build failed (retrying...): |
|
Build succeeded: |
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.
Previously, during the flow setup stage we would always choose to create
a LeafTxn for local flows if there is a processor that might use
a Streamer API. This was the case even when the Streamer is disabled by
a cluster setting. Such behavior is a regression (since leaf txns don't
have a transparent span refresh mechanism and read spans have to be
collected with the metadata at the end of the execution), so this commit
fixes things by only using a LeafTxn if the Streamer is actually
enabled. Now, the users of the Streamer API are expected to check that
they do have a LeafTxn and only use the Streamer if so.
Additionally, this commit correctly populates
HasConcurrencyfield forthe local flows even when the query might be distributed. The fix is
needed since the physical planning decision to distribute the query or
not is finalized after we decide which txn to use for the flow, which is
too late. Thus, whenever the Streamer is enabled and we find an index
join in the plan, we will make sure to use the LeafTxn (this was already
the case if the plan ended up being distributed).
Release note: None