Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-21.1: sql: properly synchronize the internal executor iterator #62923

Merged
merged 5 commits into from
Apr 9, 2021

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Apr 1, 2021

Backport 3/3 commits from #62581.
Backport 1/1 commits from #62962.
Backport 1/1 commits from #63010.

/cc @cockroachdb/release


sql: fix an edge case in the internal executor

SetColumns contract allows for the argument to be nil, yet the
iterator of the streaming internal executor expects that column schema,
if set, is non-nil. I don't think this could happen in practice, but
theoretically previously we could encounter an assertion error due to
none of the four fields in ieIteratorResult object being non-nil, and
now this is fixed.

Release note: None

sql: add context to IncrementRowsAffected interface

Release note: None

sql: properly synchronize the internal executor iterator

We recently merged an update to the internal executor to make it
streaming. Currently it is implemented by having two goroutines (the
iterator, "the consumer"; and the connExecutor, "the producer"). The
communication between them is done on the buffered channel. As a result,
both goroutines can run concurrently.

However, a crucial point was overlooked - our kv.Txns cannot be used
concurrently. Imagine a plan that reads from two tables each of which is
populated via the internal executor: if we read from the first, and then
from the second concurrently, we will have the concurrent usage of the
txn for that plan.

This commit the problem by carving out a new abstraction to optionally
synchronize the execution of the internal executor with its corresponding
iterator. The abstraction comes in sync and async flavors. In the sync
form, the ieResultChannel ensures that the reader and writer do not
concurrently operate, and, additionally provides a mechanism whereby the
reader may ensure that the writer observes an error when its attempts to
publish the previous row returns. This last point is critical to ensure
that transactions are not erroneously used after it has become unsafe.

The async flavor is still used by the internal executor when it doesn't
return an iterator directly and executes the query to completion itself.

Fixes: #62415.

Release note: None (no stable release with this bug).

sql: fix bug in close of ieSyncResultChannel

When finishing sending on the ieSyncResultChannel we do not need to and should
not close the channel used to unblock the sender. Doing so can result in a
panic if the reader is concurrently trying to unblock the sender. We could
close that channel but there's no obvious reason to.

Fixes #62939

Release note: None

sql: clean up ieResultChannel concepts and fix race condition

The async and sync implementations were too close to justify two structs.
Also, the async behavior of not stopping the writer in case the reader
called close wasn't desireable. This commit unifies the implementation.
It also ensures that we propagate context errors in all cases triggered
by the closure of the done channel. It also makes closing the channel
idempotent.

Additionally, this commit transitions the execution flow into draining
state without setting our customer error on the resultWriter.

Fixes #62948

Release note: None

@yuzefovich yuzefovich requested review from ajwerner, a team and adityamaru and removed request for a team April 1, 2021 01:11
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@yuzefovich yuzefovich removed the request for review from adityamaru April 1, 2021 19:03
@yuzefovich
Copy link
Member Author

@ajwerner I picked up the fix from #62962 into this PR too.

yuzefovich and others added 5 commits April 8, 2021 17:27
`SetColumns` contract allows for the argument to be nil, yet the
iterator of the streaming internal executor expects that column schema,
if set, is non-nil. I don't think this could happen in practice, but
theoretically previously we could encounter an assertion error due to
none of the four fields in `ieIteratorResult` object being non-nil, and
now this is fixed.

Release note: None
We recently merged an update to the internal executor to make it
streaming. Currently it is implemented by having two goroutines (the
iterator, "the consumer"; and the connExecutor, "the producer"). The
communication between them is done on the buffered channel. As a result,
both goroutines can run concurrently.

However, a crucial point was overlooked - our `kv.Txn`s cannot be used
concurrently. Imagine a plan that reads from two tables each of which is
populated via the internal executor: if we read from the first, and then
from the second concurrently, we will have the concurrent usage of the
txn for that plan.

This commit the problem by carving out a new abstraction to optionally
synchronize the execution of the internal executor with its corresponding
iterator. The abstraction comes in sync and async flavors. In the sync
form, the ieResultChannel ensures that the reader and writer do not
concurrently operate, and, additionally provides a mechanism whereby the
reader may ensure that the writer observes an error when its attempts to
publish the previous row returns. This last point is critical to ensure
that transactions are not erroneously used after it has become unsafe.

The async flavor is still used by the internal executor when it doesn't
return an iterator directly and executes the query to completion itself.

Release note: None (no stable release with this bug).
When finishing sending on the ieSyncResultChannel we do not need to and should
not close the channel used to unblock the sender. Doing so can result in a
panic if the reader is concurrently trying to unblock the sender. We could
close that channel but there's no obvious reason to.

Fixes cockroachdb#62939

Release note: None
The async and sync implementations were too close to justify two structs.
Also, the async behavior of not stopping the writer in case the reader
called close wasn't desireable. This commit unifies the implementation.
It also ensures that we propagate context errors in all cases triggered
by the closure of the done channel. It also makes closing the channel
idempotent.

Additionally, this commit transitions the execution flow into draining
state without setting our customer error on the resultWriter.

Release note: None
@yuzefovich yuzefovich force-pushed the backport21.1-62581 branch from 01622ca to 80352be Compare April 9, 2021 00:27
@yuzefovich
Copy link
Member Author

@ajwerner the fix to DistSQLReceiver has been merged and I cherry-picked all streaming internal executor fixes into this PR. RFAL.

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@yuzefovich
Copy link
Member Author

Thanks a lot Andrew for working on this with me!

@yuzefovich yuzefovich merged commit 9bd9b3a into cockroachdb:release-21.1 Apr 9, 2021
@yuzefovich yuzefovich deleted the backport21.1-62581 branch April 9, 2021 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants