colexec: protect columnarizer when closing not started input #91446
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 commit makes sure that the columnarizer calls `InternalClose` only if it has been initialized. Previously, if `Columnarizer.Init` wasn't performed (most likely due to a panic in `Init` of another operator), the columnarizer's input would not be started, so when `InternalClose` called `input.ConsumerClosed`, that could lead to a nil pointer panic since `input.Ctx` would be `nil` if the input tried to do some logging (some processors do that). We now protect against this by short-circuiting `InternalClose` call altogether, similar to what we do in `Columnarizer.DrainMeta`. This makes it so that the columnarizer satisfies `Closer.Close` contract properly. Release note: None
|
This change is |
cucaroach
approved these changes
Nov 7, 2022
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)
|
TFTR! bors r+ |
|
Build succeeded: |
This was referenced Nov 8, 2022
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Nov 16, 2022
This commit audits all usages of `ProcessorBaseNoHelper.Ctx` by all processors in their "close" methods and ensures that we never access a `nil` context. This is achieved by providing a "safe" accessor method that returns the background context should `Ctx` field be `nil` (which can occur if `StartInternal` hasn't been called). This change makes it so that all processors now respect the contract of `colexecop.Closer` interface which says that `Close` must be safe to call even if `Init` hasn't been performed (in the context of processors this means that `Columnarizer.Init` wasn't called meaning that `Processor.Start` wasn't either). Initially, I attempted to fix this in cockroachdb#91446 by putting the protection into the columnarizer, but that led to broken assumptions since we wouldn't close all closers that we expected to (in particular, the materializer that is the input to the wrapped row-by-row processor wouldn't be closed). This commit takes a different approach and should fix the issue for good without introducing any flakiness. As a result, this commit fixes a rarely hit issue when the aggregator and the zigzag joiner attempt to log when they are closed if they haven't been started (that we see occasionally from sentry). The issue is quite rare though, so no release note seems appropriate. Release note: None
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Nov 17, 2022
This commit replaces all usages of `ProcessorBaseNoHelper.Ctx` field with a call to the newly-introduced `Ctx()` method which returns a background context if the processor hasn't been started. This change makes it so that all processors now respect the contract of `colexecop.Closer` interface which says that `Close` must be safe to call even if `Init` hasn't been performed (in the context of processors this means that `Columnarizer.Init` wasn't called meaning that `Processor.Start` wasn't either). Initially, I attempted to fix this in cockroachdb#91446 by putting the protection into the columnarizer, but that led to broken assumptions since we wouldn't close all closers that we expected to (in particular, the materializer that is the input to the wrapped row-by-row processor wouldn't be closed). This commit takes a different approach and should fix the issue for good without introducing any flakiness. As a result, this commit fixes a rarely hit issue when the aggregator and the zigzag joiner attempt to log when they are closed if they haven't been started (that we see occasionally from sentry). The issue is quite rare though, so no release note seems appropriate. Release note: None
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Nov 22, 2022
This commit replaces all usages of `ProcessorBaseNoHelper.Ctx` field with a call to the newly-introduced `Ctx()` method which returns a background context if the processor hasn't been started. This change makes it so that all processors now respect the contract of `colexecop.Closer` interface which says that `Close` must be safe to call even if `Init` hasn't been performed (in the context of processors this means that `Columnarizer.Init` wasn't called meaning that `Processor.Start` wasn't either). Initially, I attempted to fix this in cockroachdb#91446 by putting the protection into the columnarizer, but that led to broken assumptions since we wouldn't close all closers that we expected to (in particular, the materializer that is the input to the wrapped row-by-row processor wouldn't be closed). This commit takes a different approach and should fix the issue for good without introducing any flakiness. As a result, this commit fixes a rarely hit issue when the aggregator and the zigzag joiner attempt to log when they are closed if they haven't been started (that we see occasionally from sentry). The issue is quite rare though, so no release note seems appropriate. Release note: None
craig bot
pushed a commit
that referenced
this pull request
Nov 22, 2022
91969: sql: audit all processors to make their closure bullet-proof r=yuzefovich a=yuzefovich This commit replaces all usages of `ProcessorBaseNoHelper.Ctx` field with a call to the newly-introduced `Ctx()` method which returns a background context if the processor hasn't been started. This change makes it so that all processors now respect the contract of `colexecop.Closer` interface which says that `Close` must be safe to call even if `Init` hasn't been performed (in the context of processors this means that `Columnarizer.Init` wasn't called meaning that `Processor.Start` wasn't either). Initially, I attempted to fix this in #91446 by putting the protection into the columnarizer, but that led to broken assumptions since we wouldn't close all closers that we expected to (in particular, the materializer that is the input to the wrapped row-by-row processor wouldn't be closed). This commit takes a different approach and should fix the issue for good without introducing any flakiness. As a result, this commit fixes a rarely hit issue when the aggregator and the zigzag joiner attempt to log when they are closed if they haven't been started (that we see occasionally from sentry). The issue is quite rare though, so no release note seems appropriate. Fixes: #84902. Fixes: #91845. Release note: None 92265: kvconnectorccl: allow secondary tenants to prefetch range lookups r=ajwerner a=ajwerner This patch permits the tenant connector to request more than 0 ranges to be prefetched. In order to enable this, we add logic in the implementation of the RangeLookup RPC to filter any results which are not intended for this tenant. Fixes #91433 Release note: None 92284: ui: show stmt idle time in execution/planning chart r=matthewtodd a=matthewtodd Part of #86667. The example statement fingerprint below comes from me connecting to a local cockroach instance with `psql` (since `cockroach sql` by default runs a few extra queries that confuse the timings) and individually running the following lines to simulate some inter-statement latency: ```sql BEGIN; SELECT 1; COMMIT; ``` | Before | After | |--|--| |<img width="1372" alt="Screen Shot 2022-11-21 at 2 35 49 PM" src="https://user-images.githubusercontent.com/5261/203144731-fd5a42fb-7b60-473a-990a-fb5fabf2756a.png">|<img width="1372" alt="Screen Shot 2022-11-21 at 2 35 58 PM" src="https://user-images.githubusercontent.com/5261/203144736-2600c0f9-7811-4b9d-a9e1-dbb8aeea71ee.png">| Release note (ui change): The "Statement Execution and Planning Time" chart on the statement fingerprint page now includes a third value, "Idle," representing the time spent by the application waiting to execute this statement while holding a transaction open. Co-authored-by: Yahor Yuzefovich <[email protected]> Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Matthew Todd <[email protected]>
![https://user-images.githubusercontent.com/5261/203144731-fd5a42fb-7b60-473a-990a-fb5fabf2756a.png">|<img](https://user-images.githubusercontent.com/5261/203144731-fd5a42fb-7b60-473a-990a-fb5fabf2756a.png">|<img){kind=link}
![https://user-images.githubusercontent.com/5261/203144736-2600c0f9-7811-4b9d-a9e1-dbb8aeea71ee.png">|](https://user-images.githubusercontent.com/5261/203144736-2600c0f9-7811-4b9d-a9e1-dbb8aeea71ee.png">|){kind=link}
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Nov 22, 2022
This commit replaces all usages of `ProcessorBaseNoHelper.Ctx` field with a call to the newly-introduced `Ctx()` method which returns a background context if the processor hasn't been started. This change makes it so that all processors now respect the contract of `colexecop.Closer` interface which says that `Close` must be safe to call even if `Init` hasn't been performed (in the context of processors this means that `Columnarizer.Init` wasn't called meaning that `Processor.Start` wasn't either). Initially, I attempted to fix this in cockroachdb#91446 by putting the protection into the columnarizer, but that led to broken assumptions since we wouldn't close all closers that we expected to (in particular, the materializer that is the input to the wrapped row-by-row processor wouldn't be closed). This commit takes a different approach and should fix the issue for good without introducing any flakiness. As a result, this commit fixes a rarely hit issue when the aggregator and the zigzag joiner attempt to log when they are closed if they haven't been started (that we see occasionally from sentry). The issue is quite rare though, so no release note seems appropriate. Release note: None
yuzefovich
added a commit
to yuzefovich/cockroach
that referenced
this pull request
Nov 22, 2022
This commit replaces all usages of `ProcessorBaseNoHelper.Ctx` field with a call to the newly-introduced `Ctx()` method which returns a background context if the processor hasn't been started. This change makes it so that all processors now respect the contract of `colexecop.Closer` interface which says that `Close` must be safe to call even if `Init` hasn't been performed (in the context of processors this means that `Columnarizer.Init` wasn't called meaning that `Processor.Start` wasn't either). Initially, I attempted to fix this in cockroachdb#91446 by putting the protection into the columnarizer, but that led to broken assumptions since we wouldn't close all closers that we expected to (in particular, the materializer that is the input to the wrapped row-by-row processor wouldn't be closed). This commit takes a different approach and should fix the issue for good without introducing any flakiness. As a result, this commit fixes a rarely hit issue when the aggregator and the zigzag joiner attempt to log when they are closed if they haven't been started (that we see occasionally from sentry). The issue is quite rare though, so no release note seems appropriate. Release note: None
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 makes sure that the columnarizer calls
InternalCloseonly if it has been initialized. Previously, ifColumnarizer.Initwasn't performed (most likely due to a panic inInitof another operator), the columnarizer's input would not be started, so whenInternalClosecalledinput.ConsumerClosed, that could lead to a nil pointer panic sinceinput.Ctxwould benilif the input tried to do some logging (some processors do that). We now protect against this by short-circuitingInternalClosecall altogether, similar to what we do inColumnarizer.DrainMeta. This makes it so that the columnarizer satisfiesCloser.Closecontract properly.Fixes: #84902.
Release note: None