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

sql: remove parallel statement execution #35959

Merged

Conversation

Projects
None yet
4 participants
@nvanbenschoten
Copy link
Member

commented Mar 19, 2019

Fixes #34789.
Closes #17504.
Closes #18226.
Closes #35780.

This commit removes all logic supporting parallel statement execution.
It does so following the removal plan described in #34789. There is only
one deviation from the plan, which is that we don't do anything to
prevent statements with RETURNING NOTHING syntax from returning valid
results. Specifically, statements with RETURNING NOTHING may return
the actual number of rows affected instead of the dummy value of 1. It's
highly unlikely that anyone was relying on this behavior and it wasn't
documented.

@nvanbenschoten nvanbenschoten requested a review from jordanlewis Mar 19, 2019

@nvanbenschoten nvanbenschoten requested review from cockroachdb/sql-async-prs as code owners Mar 19, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

This change is Reviewable

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten:nvanbenschoten/killParallelStmts branch from 56b595b to eb6c64d Mar 19, 2019

@jordanlewis
Copy link
Member

left a comment

:lgtm_strong:

Reviewed 2 of 2 files at r1, 120 of 120 files at r2, 5 of 5 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @nvanbenschoten)


pkg/sql/values.go, line 154 at r2 (raw file):

	if n.rows != nil {
		if !n.isConst {
			log.Fatalf(params.ctx, "valuesNode evaluated twice")

Hmm, I'm not sure about deleting this. valuesNode being evaluated twice is still a bad thing.

@nvanbenschoten
Copy link
Member Author

left a comment

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


pkg/sql/values.go, line 154 at r2 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Hmm, I'm not sure about deleting this. valuesNode being evaluated twice is still a bad thing.

The isConst stuff was always pretty messy. It was added in 1a71f80 so that we could evaluate valuesNodes during span analysis. Now that we ripped span analysis out, I don't think we need it. Are there other reasons that a valuesNode would be evaluated twice?

@jordanlewis

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Bugs. I'd just prefer to see that Fatal left in unconditionally.

@knz
Copy link
Member

left a comment

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


pkg/sql/values.go, line 154 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

The isConst stuff was always pretty messy. It was added in 1a71f80 so that we could evaluate valuesNodes during span analysis. Now that we ripped span analysis out, I don't think we need it. Are there other reasons that a valuesNode would be evaluated twice?

If you keep the path, please change this to return pgerror.NewAssertionErrorf(....). This way we're not crashing a node for something that merely deserves a client close.


pkg/sql/values.go, line 195 at r2 (raw file):

func (n *valuesNode) Reset(ctx context.Context) {
	if !n.isConst {
		log.Fatalf(ctx, "valuesNode.Reset can only be called on constant valuesNodes")

ditto

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten:nvanbenschoten/killParallelStmts branch from eb6c64d to 90caf12 Apr 1, 2019

@nvanbenschoten
Copy link
Member Author

left a comment

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


pkg/sql/values.go, line 154 at r2 (raw file):

Previously, knz (kena) wrote…

If you keep the path, please change this to return pgerror.NewAssertionErrorf(....). This way we're not crashing a node for something that merely deserves a client close.

Done.

@knz

knz approved these changes Apr 19, 2019

Copy link
Member

left a comment

Reviewed 1 of 120 files at r5.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis and @knz)

@knz

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Thank you!

This is good to go, moreover @BramGruneir needs it for an ongoing project.

nvanbenschoten added some commits Mar 19, 2019

sql: remove parallel statement execution
Fixes #34789.
Closes #17504.
Closes #18226.
Closes #35780.

This commit removes all logic supporting parallel statement execution.
It does so following the removal plan described in #34789. There is only
one deviation from the plan, which is that we don't do anything to
prevent statements with `RETURNING NOTHING` syntax from returning valid
results. Specifically, statements with `RETURNING NOTHING` may return
the actual number of rows affected instead of the dummy value of 1. It's
highly unlikely that anyone was relying on this behavior and it wasn't
documented.

Release note (sql change): Parallel statement execution has been removed
because the performance improvement it provided was absorbed by transactional
pipelining, which doesn't require client buy in. The RETURNING NOTHING syntax
will still be accepted but becomes a no-op.
sql: s/NewAssertionError/AssertionFailedf/
Fallout from fb8e85d.

Release note: None

@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten:nvanbenschoten/killParallelStmts branch from 90caf12 to d03ad96 Apr 22, 2019

@nvanbenschoten nvanbenschoten requested review from cockroachdb/core-prs as code owners Apr 22, 2019

@nvanbenschoten

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2019

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request Apr 22, 2019

Merge #35959
35959: sql: remove parallel statement execution r=nvanbenschoten a=nvanbenschoten

Fixes #34789.
Closes #17504.
Closes #18226.
Closes #35780.

This commit removes all logic supporting parallel statement execution.
It does so following the removal plan described in #34789. There is only
one deviation from the plan, which is that we don't do anything to
prevent statements with `RETURNING NOTHING` syntax from returning valid
results. Specifically, statements with `RETURNING NOTHING` may return
the actual number of rows affected instead of the dummy value of 1. It's
highly unlikely that anyone was relying on this behavior and it wasn't
documented.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Apr 22, 2019

Build succeeded

@craig craig bot merged commit d03ad96 into cockroachdb:master Apr 22, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@nvanbenschoten nvanbenschoten deleted the nvanbenschoten:nvanbenschoten/killParallelStmts branch Apr 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.