execinfra: correctly reset the eval.Context in InternalClose #89093
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
internal-close-ctx
branch
2 times, most recently
from
951e7e8
to
e3eb5bc
Compare
This was referenced Sep 30, 2022
DrewKimball
approved these changes
Oct 1, 2022
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @msirek and @yuzefovich)
-- commits line 8 at r1:
[nit] be -> to be
This commit fixes the way we restore the `eval.Context.Context` in `ProcessorBase.InternalClose`. Whenever a processor is started with `StartInternal`, we derive a tracing span, and in order for all things related to that processor (including the evaluation of the builtin functions) to be correctly attributed to the tracing span of the processor, we update the eval context with the new context. Most processors get a fresh copy of the eval context via `FlowCtx.NewEvalCtx`, but a couple (that are deemed to be "safe") do not do that (since they don't actually evaluate any expressions) and, instead, reuse the "main" eval context. Then, when the processor is closed, we want to reset the eval context to its original form, and we had a bug here - we would use the context that was passed into `StartInternal` rather than the "original" one, so we could leak the context with an already finished tracing span outside. This is now fixed. This bug could occur for example when we have a `FlowCoordinator` (which doesn't create a copy of the eval context) at the root of the main query and then there are some postqueries - the eval context used by the postqueries would refer to the context from the main query with a finished tracing span. However, the bug was mitigated by explicitly setting the context before running each postquery (due to an unrelated old issue). One could argue that since the processors modify the eval context, they should always use a copy of the eval context. That would be unfortunate since it would lead to increased allocations with the only modification being this context-mangering business. The fix in this commit seems more appropriate (both philosophically speaking and performance-wise). Release note: None
yuzefovich
force-pushed
the
internal-close-ctx
branch
from
e3eb5bc
to
2aef9c7
Compare
yuzefovich
commented
Oct 2, 2022
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
Previously, DrewKimball (Drew Kimball) wrote…
[nit]
be->to be
Done.
|
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.
This commit fixes the way we restore the
eval.Context.ContextinProcessorBase.InternalClose. Whenever a processor is started withStartInternal, we derive a tracing span, and in order for all thingsrelated to that processor (including the evaluation of the builtin
functions) to be correctly attributed to the tracing span of the processor,
we update the eval context with the new context. Most processors get
a fresh copy of the eval context via
FlowCtx.NewEvalCtx, but a couple(that are deemed to be "safe") do not do that (since they don't actually
evaluate any expressions) and, instead, reuse the "main" eval context.
Then, when the processor is closed, we want to reset the eval context to
its original form, and we had a bug here - we would use the context that
was passed into
StartInternalrather than the "original" one, so wecould leak the context with an already finished tracing span outside.
This is now fixed.
This bug could occur for example when we have a
FlowCoordinator(whichdoesn't create a copy of the eval context) at the root of the main query
and then there are some postqueries - the eval context used by the
postqueries would refer to the context from the main query with
a finished tracing span. However, the bug was mitigated by explicitly
setting the context before running each postquery (due to an unrelated
old issue).
One could argue that since the processors modify the eval context, they
should always use a copy of the eval context. That would be unfortunate
since it would lead to increased allocations with the only modification
being this context-mangering business. The fix in this commit seems more
appropriate (both philosophically speaking and performance-wise).
Release note: None