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

opt: fix zero-column groups in joins colliding #34667

Merged
merged 1 commit into from
Feb 6, 2019

Conversation

justinj
Copy link
Contributor

@justinj justinj commented Feb 6, 2019

It's fundamental to the optimizer that any two unrelated (i.e., one is
not the parent of the other) expressions in the tree have disjoint
column sets. Usually, this prevents the same relational expression from
occurring in the tree more than once, but in the case of a group with
zero columns, this is not true.

This allows the following group collision to occur:

Let A be the 0-column values node with two rows VALUES (), (),
and B be the 0-column values node with three rows VALUES (), (), ().

Then consider the following query:

(A JOIN B) UNION (B JOIN A)

During build, we add A JOIN B and B JOIN A to separate memo groups.
Then, during exploration, we apply the CommuteJoin rule to transform
A JOIN B to B JOIN A. This attempts to get interned into the same
group, but the interner finds that B JOIN A already exists in a
different group, and panics.

As a side note, a values node with no columns is not valid SQL, but it
can be constructed via the MergeProjectWithValues rule:

SELECT 1 FROM (VALUES (1))
=> MergeProjectWithValues
SELECT 1 FROM (VALUES ())

This commit fixes the problem by simply disabling these types of
explorations on groups with zero columns. This isn't a perfect fix,
since the subtle problem is still lurking, and people going forward need
to understand its implications, but it will fix the problem for the time
being.

Release note (bug fix): fixed several related panics in the optimizer
related to plan exploration.

It's fundamental to the optimizer that any two unrelated (i.e., one is
not the parent of the other) expressions in the tree have disjoint
column sets. Usually, this prevents the same relational expression from
occurring in the tree more than once, but in the case of a group with
zero columns, this is not true.

This allows the following group collision to occur:

Let A be the 0-column values node with two rows `VALUES (), ()`,
and B be the 0-column values node with three rows `VALUES (), (), ()`.

Then consider the following query:
```
(A JOIN B) UNION (B JOIN A)
```

During build, we add A JOIN B and B JOIN A to *separate memo groups*.
Then, during exploration, we apply the `CommuteJoin` rule to transform
`A JOIN B` to `B JOIN A`. This attempts to get interned into the same
group, but the interner finds that `B JOIN A` already exists in a
different group, and panics.

As a side note, a values node with no columns is not valid SQL, but it
can be constructed via the `MergeProjectWithValues` rule:
```
SELECT 1 FROM (VALUES (1))
=> MergeProjectWithValues
SELECT 1 FROM (VALUES ())
```

This commit fixes the problem by simply disabling these types of
explorations on groups with zero columns. This isn't a perfect fix,
since the subtle problem is still lurking, and people going forward need
to understand its implications, but it will fix the problem for the time
being.

Release note (bug fix): fixed several related panics in the optimizer
related to plan exploration.
@justinj justinj requested a review from a team as a code owner February 6, 2019 15:01
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@andy-kimball andy-kimball 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 (waiting on @andy-kimball, @RaduBerinde, and @rytaft)

@justinj
Copy link
Contributor Author

justinj commented Feb 6, 2019

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Feb 6, 2019
34667: opt: fix zero-column groups in joins colliding r=justinj a=justinj

It's fundamental to the optimizer that any two unrelated (i.e., one is
not the parent of the other) expressions in the tree have disjoint
column sets. Usually, this prevents the same relational expression from
occurring in the tree more than once, but in the case of a group with
zero columns, this is not true.

This allows the following group collision to occur:

Let A be the 0-column values node with two rows `VALUES (), ()`,
and B be the 0-column values node with three rows `VALUES (), (), ()`.

Then consider the following query:
```
(A JOIN B) UNION (B JOIN A)
```

During build, we add A JOIN B and B JOIN A to *separate memo groups*.
Then, during exploration, we apply the `CommuteJoin` rule to transform
`A JOIN B` to `B JOIN A`. This attempts to get interned into the same
group, but the interner finds that `B JOIN A` already exists in a
different group, and panics.

As a side note, a values node with no columns is not valid SQL, but it
can be constructed via the `MergeProjectWithValues` rule:
```
SELECT 1 FROM (VALUES (1))
=> MergeProjectWithValues
SELECT 1 FROM (VALUES ())
```

This commit fixes the problem by simply disabling these types of
explorations on groups with zero columns. This isn't a perfect fix,
since the subtle problem is still lurking, and people going forward need
to understand its implications, but it will fix the problem for the time
being.

Release note (bug fix): fixed several related panics in the optimizer
related to plan exploration.

Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Feb 6, 2019

Build succeeded

@craig craig bot merged commit 62a08f7 into cockroachdb:master Feb 6, 2019
@justinj justinj deleted the fix-collisions branch February 10, 2019 18:51
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.

None yet

3 participants