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 rule cycle bug #28848

Merged
merged 1 commit into from
Aug 20, 2018
Merged

opt: Fix rule cycle bug #28848

merged 1 commit into from
Aug 20, 2018

Conversation

andy-kimball
Copy link
Contributor

The PushFilterIntoJoinLeftAndRight and TryDecorrelateSelect rules can cycle
with one another in a rare case:

  1. Right side of join has outer column due to being un-decorrelatable.
  2. Filter conjunct is pushed down to both left and right side by mapping
    equivalencies in PushFilterIntoJoinLeftAndRight.
  3. Left conjunct is pulled back up to join condition by TryDecorrelateSelect.

Steps #2 and #3 will cycle with one another. Cycle detection is not possible
in this case, because the left side keeps changing (because new conjuct is
pushed down to it each time).

The fix is simple: don't let PushFilterIntoJoinLeftAndRight push down filters
if either the left or right side has outer column(s).

This fixes #28818.

Release note: None

The PushFilterIntoJoinLeftAndRight and TryDecorrelateSelect rules can cycle
with one another in a rare case:

1. Right side of join has outer column due to being un-decorrelatable.
2. Filter conjunct is pushed down to both left and right side by mapping
   equivalencies in PushFilterIntoJoinLeftAndRight.
3. Left conjunct is pulled back up to join condition by TryDecorrelateSelect.

Steps cockroachdb#2 and cockroachdb#3 will cycle with one another. Cycle detection is not possible
in this case, because the left side keeps changing (because new conjuct is
pushed down to it each time).

The fix is simple: don't let PushFilterIntoJoinLeftAndRight push down filters
if either the left or right side has outer column(s).

This fixes cockroachdb#28818.

Release note: None
@andy-kimball andy-kimball requested a review from a team as a code owner August 20, 2018 18:18
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@RaduBerinde RaduBerinde 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! 0 of 0 LGTMs obtained (and 1 stale)

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)


pkg/sql/opt/norm/testdata/rules/join, line 961 at r1 (raw file):


# Regression for issue 28818. Try to trigger undetectable cycle between the
# PushFilterIntoJoinLeftAndRight and TryDecorrelateSelect rules.

I'm confused - how does this trigger the rule PushFilterIntoJoinLeftAndRight? I don't see any variable equality conditions....

Copy link
Contributor Author

@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.

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


pkg/sql/opt/norm/testdata/rules/join, line 961 at r1 (raw file):

Previously, rytaft wrote…

I'm confused - how does this trigger the rule PushFilterIntoJoinLeftAndRight? I don't see any variable equality conditions....

Because (SELECT s FROM a) = 'foo' is effectively constant. It gets pushed down to both sides since it can be trivially "mapped" to either side.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

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

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


pkg/sql/opt/norm/testdata/rules/join, line 961 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

Because (SELECT s FROM a) = 'foo' is effectively constant. It gets pushed down to both sides since it can be trivially "mapped" to either side.

got it - thanks!

@andy-kimball
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Aug 20, 2018
28340: storage: make lease rebalancing decisions at the store level r=a-robinson a=a-robinson

In order to better balance the QPS being served by each store to avoid
overloaded nodes.

Fixes #21419

Release note (performance improvement): Range leases will be
automatically rebalanced throughout the cluster to even out the amount
of QPS being handled by each node.

28848: opt: Fix rule cycle bug r=andy-kimball a=andy-kimball

The PushFilterIntoJoinLeftAndRight and TryDecorrelateSelect rules can cycle
with one another in a rare case:

1. Right side of join has outer column due to being un-decorrelatable.
2. Filter conjunct is pushed down to both left and right side by mapping
   equivalencies in PushFilterIntoJoinLeftAndRight.
3. Left conjunct is pulled back up to join condition by TryDecorrelateSelect.

Steps #2 and #3 will cycle with one another. Cycle detection is not possible
in this case, because the left side keeps changing (because new conjuct is
pushed down to it each time).

The fix is simple: don't let PushFilterIntoJoinLeftAndRight push down filters
if either the left or right side has outer column(s).

This fixes #28818.

Release note: None

Co-authored-by: Alex Robinson <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 20, 2018

Build succeeded

@craig craig bot merged commit c7772ac into cockroachdb:master Aug 20, 2018
@andy-kimball andy-kimball deleted the inf branch August 20, 2018 21:26
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.

opt: hang / infinite recursion (?) in ConstructInnerJoin
4 participants