opt: nil presentation should not equal the 0-column presentation #39818
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
We have seen a series of bugs where the execbuilder hits an assertion error inside Apply. This change adds a detail to the error containing the formatted opt tree. Release note: None
|
This change is |
justinj
reviewed
Aug 22, 2019
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @justinj, @RaduBerinde, and @rytaft)
pkg/sql/opt/memo/interner.go, line 525 at r2 (raw file):
func (h *hasher) HashPhysProps(val *physical.Required) { // Note: the Any presentation is not the same with the 0-column presentation.
nit: "is not the same as"?
pkg/sql/opt/props/physical/required.go, line 118 at r2 (raw file):
// presentation. func (p Presentation) Equals(rhs Presentation) bool { // The 0 column presentation is not the same with the nil presentation.
nit: same as? or "is not equal to"
The nil presentation means that we want all columns in no particular order, whereas the 0-column presentation means that we don't need any of the columns (e.g. EXISTS subqueries). Only the nil presentation is `Any()`. But `Equals()` was incorrectly treating these as equal. This confused the interner and a nil presentation became a 0-column presentation which led to some internal errors in Apply. For completeness, we also fix `HashPhysProps` to differentiate between these two, but note that this isn't required for correctness. Fixes cockroachdb#39435. Release note (bug fix): Fixed internal errors generated during execution of some complicated cases of correlated subqueries.
RaduBerinde
commented
Aug 22, 2019
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/memo/interner.go, line 525 at r2 (raw file):
Previously, justinj (Justin Jaffray) wrote…
nit: "is not the same as"?
In Soviet Russia, English learns you!
rytaft
approved these changes
Aug 22, 2019
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 1 of 3 files at r2, 2 of 2 files at r3.
Reviewable status:
|
bors r+ |
craig bot
pushed a commit
that referenced
this pull request
Aug 23, 2019
39804: opt: add FK benchmarks r=RaduBerinde a=RaduBerinde Adding simple insert benchmarks comparing no FKs with the old FK path and the new one. The results are below. Note that we haven't yet optimized the constant input case (we can remove the buffer / scan nodes). Command line: ``` GOMAXPROCS=1 make bench PKG=./pkg/sql/opt/bench BENCHTIMEOUT=30m BENCHES='BenchmarkFKInsert/' TESTFLAGS='-logtostderr NONE -benchtime=20000x -count 5' ``` ``` name time/op FKInsert/SingleRow/None 609µs ± 4% FKInsert/SingleRow/Old 676µs ± 5% FKInsert/SingleRow/New 961µs ± 1% FKInsert/MultiRowSingleParent/None 1.37ms ± 1% FKInsert/MultiRowSingleParent/Old 2.14ms ± 1% FKInsert/MultiRowSingleParent/New 2.11ms ± 2% FKInsert/MultiRowMultiParent/None 1.62ms ± 2% FKInsert/MultiRowMultiParent/Old 2.89ms ± 1% FKInsert/MultiRowMultiParent/New 2.97ms ± 1% ``` Release note: None 39818: opt: nil presentation should not equal the 0-column presentation r=RaduBerinde a=RaduBerinde #### sql: enhance apply execbuilder error We have seen a series of bugs where the execbuilder hits an assertion error inside Apply. This change adds a detail to the error containing the formatted opt tree. Release note: None #### opt: nil presentation should not equal the 0-column presentation The nil presentation means that we want all columns in no particular order, whereas the 0-column presentation means that we don't need any of the columns (e.g. EXISTS subqueries). Only the nil presentation is `Any()`. But `Equals()` was incorrectly treating these as equal. This confused the interner and a nil presentation became a 0-column presentation which led to some internal errors in Apply. For completeness, we also fix `HashPhysProps` to differentiate between these two, but note that this isn't required for correctness. Fixes #39435. Release note (bug fix): Fixed internal errors generated during execution of some complicated cases of correlated subqueries. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Build succeeded |
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.
sql: enhance apply execbuilder error
We have seen a series of bugs where the execbuilder hits an assertion
error inside Apply. This change adds a detail to the error containing
the formatted opt tree.
Release note: None
opt: nil presentation should not equal the 0-column presentation
The nil presentation means that we want all columns in no particular
order, whereas the 0-column presentation means that we don't need any
of the columns (e.g. EXISTS subqueries). Only the nil presentation
is
Any(). ButEquals()was incorrectly treating these as equal.This confused the interner and a nil presentation became a 0-column
presentation which led to some internal errors in Apply.
For completeness, we also fix
HashPhysPropsto differentiatebetween these two, but note that this isn't required for correctness.
Fixes #39435.
Release note (bug fix): Fixed internal errors generated during
execution of some complicated cases of correlated subqueries.