flowinfra: replace ConcurrentExecution with ConcurrentTxnUse #52621
Conversation
|
This change is |
There was a problem hiding this comment.
Are there any other processors that cannot be fused but also don't use a txn?
Reviewed 13 of 13 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @asubiotto)
pkg/sql/execinfra/processorsbase.go, line 45 at r1 (raw file):
// local flow from running in parallel if this is unknown since concurrent use // of the RootTxn is forbidden (in a distributed flow these are leaf txns, so // it doesn't matter.)
nit: period before parenthesis.
Previously, the DistSQLPlanner would reject any local flows with any concurrency due to possible concurrent use of a RootTxn. This policy was overly prohibitive as it would reject stats flows that contain processors that cannot be fused even though there would be no concurrent txn usage. This commit makes this policy a bit more lenient, introducing a way for processors to indicate that they and their input will not be using a txn. This is exposed through the new ConcurrentTxnUse method in the flow interface, which returns whether more than one processor will be using a txn. This commit also removes ConcurrentExecution since all usages have been replaced with ConcurrentTxnUse. Release note: None (internal refactor to fix an error message received only in a multitenant environment)
There was a problem hiding this comment.
Probably, but it's not the goal of this PR to implement DoesNotUseTxn for all processors, only for the stats ones to fix the immediate problem of not being able to run CREATE STATS in a multitenant environment. If we encounter a problem related to other processors, the fix should be as easy as implementing DoesNotUseTxn.
Reviewable status:
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
There was a problem hiding this comment.
that cannot be fused even though there would be no concurrent txn usage
Is there a good reason why whatever prompted this cannot be fused?
Reviewable status:
|
It's another solution I mentioned in (#50049 (comment)), but it's code I'm not familiar with (stats collection processors) and will probably be non-trivial based on my experience refactoring so want to leave it on the optimizer team's plate. Created an issue: #52700 |
There was a problem hiding this comment.
Let's see what @rytaft says. If we can just fuse whatever, I'd rather do that than introduce this interface since I don't trust an interface like this to stay accurate.
Reviewable status:
|
@rytaft is OOO this week. I don't see any reason we can't fuse them, but we are all pretty swamped right now. |
|
To be clear: the inability to fuse in the commit message is in reference to the current state of the DistSQLPlanner not being able to do so because the processors do not implement I'm going to go ahead and merge this to unblock stats creation in a multitenant environment. This is an opt-in interface, and I think it's very unlikely for any processors that need to implement this interface to change their behavior to suddenly use a bors r=yuzefovich |
|
Build succeeded: |
Previously, the DistSQLPlanner would reject any local flows with any
concurrency due to possible concurrent use of a RootTxn. This policy was overly
prohibitive as it would reject stats flows that contain processors that cannot
be fused even though there would be no concurrent txn usage.
This commit makes this policy a bit more lenient, introducing a way for
processors to indicate that they and their input will not be using a txn. This
is exposed through the new ConcurrentTxnUse method in the flow interface, which
returns whether more than one processor will be using a txn.
This commit also removes ConcurrentExecution since all usages have been
replaced with ConcurrentTxnUse.
Release note: None (internal refactor to fix an error message received only in
a multitenant environment)
Fixes #50049