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

exec: protect against unset syncFlowConsumer #38557

Merged
merged 1 commit into from Jul 8, 2019

Conversation

@asubiotto
Copy link
Contributor

asubiotto commented Jun 28, 2019

This should never happen since it implies that the receiver isn't
connected correctly. These happen when a node sends a SetupFlow request
to a remote node where the spec specifies that the response is on that
remote node. We don't see panics in the row execution engine due to
wrapping the syncFlowConsumer with a copyingRowReceiver, but this state
can cause setupVectorized to panic.

This commit protects against this state pending further investigation.

Release note: None

This should never happen since it implies that the receiver isn't
connected correctly. These happen when a node sends a SetupFlow request
to a remote node where the spec specifies that the response is on that
remote node. We don't see panics in the row execution engine due to
wrapping the syncFlowConsumer with a copyingRowReceiver, but this state
can cause setupVectorized to panic.

This commit protects against this state pending further investigation.

Release note: None
@asubiotto asubiotto requested review from jordanlewis and Jun 28, 2019
@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Jun 28, 2019

This change is Reviewable

@asubiotto

This comment has been minimized.

Copy link
Contributor Author

asubiotto commented Jun 28, 2019

Looks like the ones causing these are internal executor queries:

I190628 15:59:45.676023 3672 sql/distsql_running.go:180  [intExec=adopt-job] error vectorizing remote flow d6d8146f-f896-41d8-8c17-82bae5f1bbd3, restarting with vectorize=off and ID 10d20b4f-490d-48b6-943a-a1bc791f5102: syncFlowConsumer unset, unable to create materializer
Copy link
Member

jordanlewis left a comment

:lgtm: but this seems weird for sure.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)


pkg/sql/distsqlrun/column_exec_setup.go, line 1080 at r1 (raw file):

			case distsqlpb.StreamEndpointSpec_SYNC_RESPONSE:
				if f.syncFlowConsumer == nil {
					return errors.New("syncFlowConsumer unset, unable to create materializer")

When would this practically happen? It seems like kind of a weird case. Could this be pointing at an error case we're not catching or something?

@asubiotto asubiotto requested a review from cockroachdb/sql-execution Jul 2, 2019
Copy link
Contributor Author

asubiotto left a comment

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)


pkg/sql/distsqlrun/column_exec_setup.go, line 1080 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

When would this practically happen? It seems like kind of a weird case. Could this be pointing at an error case we're not catching or something?

I190628 15:59:45.676023 3672 sql/distsql_running.go:180 [intExec=adopt-job] error vectorizing remote flow d6d8146f-f896-41d8-8c17-82bae5f1bbd3, restarting with vectorize=off and ID 10d20b4f-490d-48b6-943a-a1bc791f5102: syncFlowConsumer unset, unable to create materializer
I don't think this is an error case we're not catching, it seems like a bad plan output from the physical planner. This needs some more investigation for sure.

Copy link
Contributor Author

asubiotto left a comment

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis)


pkg/sql/distsqlrun/column_exec_setup.go, line 1080 at r1 (raw file):

Previously, asubiotto (Alfonso Subiotto Marqués) wrote…

I190628 15:59:45.676023 3672 sql/distsql_running.go:180 [intExec=adopt-job] error vectorizing remote flow d6d8146f-f896-41d8-8c17-82bae5f1bbd3, restarting with vectorize=off and ID 10d20b4f-490d-48b6-943a-a1bc791f5102: syncFlowConsumer unset, unable to create materializer
I don't think this is an error case we're not catching, it seems like a bad plan output from the physical planner. This needs some more investigation for sure.

The common theme seems to be that these things are run through an internal executor, there might be something funky going on when setting up a receiver in those cases.

@asubiotto

This comment has been minimized.

Copy link
Contributor Author

asubiotto commented Jul 8, 2019

bors r=jordanlewis

craig bot pushed a commit that referenced this pull request Jul 8, 2019
38557: exec: protect against unset syncFlowConsumer r=jordanlewis a=asubiotto

This should never happen since it implies that the receiver isn't
connected correctly. These happen when a node sends a SetupFlow request
to a remote node where the spec specifies that the response is on that
remote node. We don't see panics in the row execution engine due to
wrapping the syncFlowConsumer with a copyingRowReceiver, but this state
can cause setupVectorized to panic.

This commit protects against this state pending further investigation.

Release note: None

38654: exec: Handle NULLS in TopK sorter r=rohany a=rohany

This commit fixes NULLs in the TopK sorter by avoiding use
of the vec copy method, which has a bug. Instead, we add
a set method to the vec comparator, and use the templatized
comparator to perform the sets that the TopK sorter needs.

To facilitate this, we add an UnsetNull method to the Nulls
object. However, use of this method results in HasNull()
maybe returning true even if the vector doesn't have nulls.
This behavior already occurs when selection vectors are used.
Based on discussions with @solongordon and @asubiotto, this behavior
is OK, and future PR's will attempt to make this behavior better, and address
the bugs within the Vec Copy method.

38710: errors: fix the formatting with %+v r=knz a=knz

(found by @RaduBerinde; needed to complete #38570)

The new library `github.com/cockroachdb/errors` was not implementing
`%+v` formatting properly for assertion and unimplemented errors.
The wrong implementation was hiding the details of the cause
of these two error types from the formatting logic.

Fixing this bug comprehensively required completing the investigation
of the Go 2 / `xerrors` error proposal. This revealed that the
implementation of `fmt.Formatter` for wrapper errors (a `Format()`
method) is required in all cases, at least until Go's stdlib
learns about `errors.Formatter`. More details at
golang/go#29934 and this commit message: cockroachdb/errors@78b6caa.

This patch bumps the dependency `github.com/cockroachdb/errors` to
pick up the fixes to assertion failures and unimplemented errors.

The new definition of `errors.FormatError()` subsequently required
re-implemening `Format)` for `pgerros.withCandidateCode`, which is
also done here.

Finally, this patch also picks up `errors.As()` and the new
streamlined `fmt.Formatter` / `errors.Formatter` interaction, so this
patch also simplifies a few custom error types in CockroachDB
accordingly.

Release note: None

Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Jul 8, 2019

Build failed (retrying...)

craig bot pushed a commit that referenced this pull request Jul 8, 2019
38557: exec: protect against unset syncFlowConsumer r=jordanlewis a=asubiotto

This should never happen since it implies that the receiver isn't
connected correctly. These happen when a node sends a SetupFlow request
to a remote node where the spec specifies that the response is on that
remote node. We don't see panics in the row execution engine due to
wrapping the syncFlowConsumer with a copyingRowReceiver, but this state
can cause setupVectorized to panic.

This commit protects against this state pending further investigation.

Release note: None

38613: exec: Add support for projection of the IN operator r=rohany a=rohany

vectorized execution now supports statements of the form

```
select x, x IN (1,2,3) from t;
```

Co-authored-by: Alfonso Subiotto Marqués <alfonso@cockroachlabs.com>
Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig

This comment has been minimized.

Copy link

craig bot commented Jul 8, 2019

Build succeeded

@craig craig bot merged commit 08cc37d into cockroachdb:master Jul 8, 2019
3 checks passed
3 checks passed
GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details
@asubiotto asubiotto deleted the asubiotto:npe branch Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.