flowinfra: fix shutting down vectorized flows with wrapped processors #43579

yuzefovich · 2019-12-27T20:01:13Z

flowinfra: fix shutting down vectorized flows with wrapped processors

Our flow shutdown mechanism relies on all components to listen to the
same context which is canceled by the "root" component (either
materializer or outbox) when the query needs to exit. Columnarizers
currently capture the context that is passed into their constructor
and will pass that context further into their downstream operators.
However, I believe it is possible that the cancellation of the "flow"
context will not trigger the cancellation of the columnarizer's context,
so some components are left hanging.

The problem was that we derived a new context in startInternal call,
but it was not returned outside of FlowBase.Setup method, so
ServerImpl.setupFlow used to return an old context. Now this is fixed by
doing the context derivation as the first step of Setup and returning
it.

Fixes: #43269.

Release note (bug fix): Previously, a query shutdown mechanism could fail
to fully cleanup the infrastructure when the query was executed via the
vectorized engine and the query plan contained wrapped row-by-row
processors (in 19.2 release those are Lookup join and Index join).

cockroach-teamcity · 2019-12-27T20:01:21Z

This change is

asubiotto

Did you figure out how the context passed into the constructor differed from the expected context? Would be nice to figure that out before getting this in just to understand this problem more. Also, could you add a wrapped processor to the VectorizedFlowShutdownTest to test this scenario? I guess it's specific to how a flow is set up and the test does that itself, but might be good to have that condition anyway.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

yuzefovich

Hm, I spent some time trying to understand where the contexts deviate to no success. I came to this solution from "philosophical" grounds. I feel like there might be a place in the code where we don't use the "descendant" of the cancellation context, but I couldn't find it. One place where this could happen maybe is in the flow scheduler, I'm not very familiar with that code though.

I also spent some time thinking about how to create a unit test for this and also didn't succeed. The complication here is that if we explicitly use different contexts for wrapped processors, then we're not doing what the "production" code now will do; but if we use the same contexts, then we're not testing much. It seems like too many things to mock out. I added the logic test from the issue, and it seems to me to be the best option.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/flowinfra/flow.go, line 339 at r1 (raw file):

// Start is part of the Flow interface.
func (f *FlowBase) Start(ctx context.Context, doneFn func()) error {
	if _, err := f.startInternal(ctx, f.processors, doneFn); err != nil {

I thought that maybe this spot was the culprit, so I tried returning new context from Start method - unfortunately, that didn't work.

asubiotto

Isn't the problem the fact that we only derive a cancellable context in startInternal but are very likely to use a parent context in Setup since none is returned from Setup? How about rather than using SetupCtxCancel, we derive a cancellable context in Setup, store that in the flow, and then do the same in startInternal, then set ctxCancel to cancel both?

Reviewable status: complete! 0 of 0 LGTMs obtained

yuzefovich

So you're suggesting having two ctxCancels in FlowBase? That would also mean two ctxDones.

Reviewable status: complete! 0 of 0 LGTMs obtained

yuzefovich

Oh, I think I see the problem.

Reviewable status: complete! 0 of 0 LGTMs obtained

asubiotto · 2020-01-13T21:19:02Z

I'm suggesting having a setupCtxCancel and setupCtxDone that is set in Setup and then in startInternal:

ctx, cancel = WithCancel(startInternalCtx)
flow.ctxCancel = func () {
    flow.setupCtxCancel()
    cancel()
    <-flow.setupCtxDone
}
flow.ctxDone = ctx.Done()

Or something like that

yuzefovich

Yep, that was it. PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained

pkg/sql/distsql/server.go, line 352 at r3 (raw file):

	}
	var err error
	if ctx, err = f.Setup(ctx, &req.Flow, opt); err != nil {

This was the actual problem - we were not returning the correct context from ServerImpl.setupFlow.

asubiotto

Reviewed 1 of 1 files at r2, 4 of 5 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained

Our flow shutdown mechanism relies on all components to listen to the same context which is canceled by the "root" component (either materializer or outbox) when the query needs to exit. Columnarizers currently capture the context that is passed into their constructor and will pass that context further into their downstream operators. However, I believe it is possible that the cancellation of the "flow" context will not trigger the cancellation of the columnarizer's context, so some components are left hanging. The problem was that we derived a new context in `startInternal` call, but it was not returned outside of `FlowBase.Setup` method, so ServerImpl.setupFlow used to return an old context. Now this is fixed by doing the context derivation as the first step of `Setup` and returning it. Release note (bug fix): Previously, a query shutdown mechanism could fail to fully cleanup the infrastructure when the query was executed via the vectorized engine and the query plan contained wrapped row-by-row processors (in 19.2 release those are Lookup join and Index join).

yuzefovich · 2020-01-13T22:31:20Z

TFTR!

bors r+

craig · 2020-01-13T22:52:05Z

Build failed

GitHub CI (Cockroach)

yuzefovich · 2020-01-13T22:52:49Z

Flake.

bors r+

43579: flowinfra: fix shutting down vectorized flows with wrapped processors r=yuzefovich a=yuzefovich flowinfra: fix shutting down vectorized flows with wrapped processors Our flow shutdown mechanism relies on all components to listen to the same context which is canceled by the "root" component (either materializer or outbox) when the query needs to exit. Columnarizers currently capture the context that is passed into their constructor and will pass that context further into their downstream operators. However, I believe it is possible that the cancellation of the "flow" context will not trigger the cancellation of the columnarizer's context, so some components are left hanging. The problem was that we derived a new context in `startInternal` call, but it was not returned outside of `FlowBase.Setup` method, so ServerImpl.setupFlow used to return an old context. Now this is fixed by doing the context derivation as the first step of `Setup` and returning it. Fixes: #43269. Release note (bug fix): Previously, a query shutdown mechanism could fail to fully cleanup the infrastructure when the query was executed via the vectorized engine and the query plan contained wrapped row-by-row processors (in 19.2 release those are Lookup join and Index join). Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

craig · 2020-01-13T23:40:16Z

Build succeeded

GitHub CI (Cockroach)

yuzefovich requested review from asubiotto and a team December 27, 2019 20:01

yuzefovich added the do-not-merge bors won't merge a PR with this label. label Dec 27, 2019

yuzefovich force-pushed the vec-shutdown branch 2 times, most recently from 8842c24 to ce6a6f7 Compare December 28, 2019 02:08

yuzefovich changed the title ~~colexec: fix shutting down remote flows with wrapped processors~~ colexec: fix shutting down flows with wrapped processors Dec 28, 2019

yuzefovich force-pushed the vec-shutdown branch from ce6a6f7 to eb256b1 Compare December 30, 2019 22:03

yuzefovich removed the do-not-merge bors won't merge a PR with this label. label Dec 30, 2019

yuzefovich changed the title ~~colexec: fix shutting down flows with wrapped processors~~ flowinfra: fix shutting down vectorized flows with wrapped processors Dec 30, 2019

yuzefovich added the backport-19.2.x Flags PRs that need to be backported to 19.2. label Dec 30, 2019

yuzefovich force-pushed the vec-shutdown branch from eb256b1 to 2b4ea79 Compare December 30, 2019 23:20

asubiotto reviewed Jan 13, 2020

View reviewed changes

yuzefovich force-pushed the vec-shutdown branch from 2b4ea79 to d246e71 Compare January 13, 2020 19:39

yuzefovich commented Jan 13, 2020

View reviewed changes

asubiotto reviewed Jan 13, 2020

View reviewed changes

yuzefovich commented Jan 13, 2020

View reviewed changes

yuzefovich force-pushed the vec-shutdown branch from d246e71 to 777b798 Compare January 13, 2020 21:26

yuzefovich commented Jan 13, 2020

View reviewed changes

asubiotto approved these changes Jan 13, 2020

View reviewed changes

yuzefovich force-pushed the vec-shutdown branch from 777b798 to d5b2f37 Compare January 13, 2020 22:05

craig bot merged commit d5b2f37 into cockroachdb:master Jan 13, 2020

yuzefovich mentioned this pull request Jan 13, 2020

release-19.2: flowinfra: fix shutting down vectorized flows with wrapped processors #43944

Merged

yuzefovich deleted the vec-shutdown branch January 13, 2020 23:55

yuzefovich mentioned this pull request Jan 14, 2020

roachtest: tpchvec failed #43574

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flowinfra: fix shutting down vectorized flows with wrapped processors #43579

flowinfra: fix shutting down vectorized flows with wrapped processors #43579

yuzefovich commented Dec 27, 2019 •

edited

cockroach-teamcity commented Dec 27, 2019

asubiotto left a comment

yuzefovich left a comment

asubiotto left a comment

yuzefovich left a comment

yuzefovich left a comment

asubiotto commented Jan 13, 2020

yuzefovich left a comment

asubiotto left a comment

yuzefovich commented Jan 13, 2020

craig bot commented Jan 13, 2020

yuzefovich commented Jan 13, 2020

craig bot commented Jan 13, 2020

flowinfra: fix shutting down vectorized flows with wrapped processors #43579

flowinfra: fix shutting down vectorized flows with wrapped processors #43579

Conversation

yuzefovich commented Dec 27, 2019 • edited

cockroach-teamcity commented Dec 27, 2019

asubiotto left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

asubiotto commented Jan 13, 2020

yuzefovich left a comment

Choose a reason for hiding this comment

asubiotto left a comment

Choose a reason for hiding this comment

yuzefovich commented Jan 13, 2020

craig bot commented Jan 13, 2020

Build failed

yuzefovich commented Jan 13, 2020

craig bot commented Jan 13, 2020

Build succeeded

yuzefovich commented Dec 27, 2019 •

edited