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.

Sign up for GitHub

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

Jump to bottom

opt: add more cost for lookup joins with more ON conditions #35587

Merged
merged 1 commit into from
Mar 12, 2019
Merged

opt: add more cost for lookup joins with more ON conditions #35587

merged 1 commit into from
Mar 12, 2019

Conversation

RaduBerinde
Copy link
Member

This is a very limited fix for #34810.

The core problem is that we don't take into account that if we have an
ON condition, not only there's a cost to evaluate it on each row, but
we are generating more internal rows to get a given number of output
rows.

I attempted to do a more general fix (for all join types), where I
tried to estimate the "internal" number of rows using
unknownFilterSelectivity for each ON condition. There were two
problems:

  • in some cases (especially with lookup joins) we have an extra ON
    condition that doesn't actually do anything:
    ab JOIN xy ON a=x AND a=10 becomes
    ab JOIN xy ON a=x AND a=10 AND x=10 becomes
    and a=10 could remain as an ON condition. This results in bad
    query plans in important cases (e.g. TPCC) where it prefers to do
    an extra lookup join (due to a non-covering index) just because of
    that condition.

  • we don't have the equality columns readily available for hash join
    (and didn't want to extract them each time we cost). In the future
    we may split the planning into a logical and physical stage, and we
    should then separate the logical joins from hash join.

For 19.1, we simply simply add a cost for lookup joins that is
proportional to the number of remaining ON conditions. This is the
least disruptive method that still fixes the case observed in #34810.
I will leave the issue open to address this properly in the next
release.

Note that although hash joins and merge joins have the same issue in
principle, in practice we always generate these expressions with
equality on all possible columns.

Release note: None

@RaduBerinde RaduBerinde requested a review from a team as a code owner March 10, 2019 13:29
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde
Copy link
Member Author

@justinj - I wonder if/how much this affects the measurements in #35561.

andy-kimball
andy-kimball approved these changes Mar 11, 2019
View reviewed changes
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, @justinj, and @RaduBerinde)

pkg/sql/opt/xform/testdata/coster/join, line 173 at r1 (raw file):

exec-ddl
CREATE TABLE abcde (
	a TEXT NOT NULL,

NIT: tabs => spaces

justinj
justinj reviewed Mar 11, 2019
View reviewed changes
Copy link
Contributor

@justinj justinj left a comment

Choose a reason for hiding this comment

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

:lgtm:, there's no leftover filter in the query I tested there so I don't think it will affect it at all.

Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @justinj and @RaduBerinde)

@RaduBerinde
Copy link
Member Author

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 11, 2019

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Mar 11, 2019

Build failed (retrying...)

@craig
Copy link
Contributor

craig bot commented Mar 11, 2019

Build failed (retrying...)

@danhhz
Copy link
Contributor

danhhz commented Mar 11, 2019

I think this may have a merge skew with master. TestCoster/zone has been failing in the bors builds

@RaduBerinde
Copy link
Member Author

Thanks!

bors r-

@craig
Copy link
Contributor

craig bot commented Mar 11, 2019

Canceled

@RaduBerinde
opt: add more cost for lookup joins with more ON conditions
ceba034 
This is a very limited fix for cockroachdb#34810.

The core problem is that we don't take into account that if we have an
ON condition, not only there's a cost to evaluate it on each row, but
we are generating more internal rows to get a given number of output
rows.

I attempted to do a more general fix (for all join types), where I
tried to estimate the "internal" number of rows using
`unknownFilterSelectivity` for each ON condition. There were two
problems:
 - in some cases (especially with lookup joins) we have an extra ON
   condition that doesn't actually do anything:
     `ab JOIN xy ON a=x AND a=10` becomes
     `ab JOIN xy ON a=x AND a=10 AND x=10` becomes
   and `a=10` could remain as an ON condition. This results in bad
   query plans in important cases (e.g. TPCC) where it prefers to do
   an extra lookup join (due to a non-covering index) just because of
   that condition.

 - we don't have the equality columns readily available for hash join
   (and didn't want to extract them each time we cost). In the future
   we may split the planning into a logical and physical stage, and we
   should then separate the logical joins from hash join.

For 19.1, we simply simply add a cost for lookup joins that is
proportional to the number of remaining ON conditions. This is the
least disruptive method that still fixes the case observed in cockroachdb#34810.
I will leave the issue open to address this properly in the next
release.

Note that although hash joins and merge joins have the same issue in
principle, in practice we always generate these expressions with
equality on all possible columns.

Release note: None
@RaduBerinde
Copy link
Member Author

bors r+

1 similar comment
@RaduBerinde
Copy link
Member Author

bors r+

craig bot pushed a commit that referenced this pull request Mar 12, 2019
@rytaft @RaduBerinde
@nvanbenschoten @celiala
Merge #35321 #35587 #35630 #35644 #35651
e44f517 
35321: opt: propagate set operation output types to input columns r=rytaft a=rytaft

This commit updates the `optbuilder` logic for set operations in which
the types of the input columns do not match the types of the output
columns. This can happen if a column on one side has type Unknown,
but the corresponding column on the other side has a known type such
as Int. The known type must be propagated to the side with the unknown
type to prevent errors in the execution engine related to decoding
types.

If there are any column types on either side that don't match the output,
then the `optbuilder` propagates the output types of the set operation down
to the input columns by wrapping the side with mismatched types in a
Project operation. The Project operation passes through columns that
already have the correct type, and creates cast expressions for those
that don't.

Fixes #34524

Release note (bug fix): Fixed an error that happened when executing
some set operations containing only nulls in one of the input columns.

35587: opt: add more cost for lookup joins with more ON conditions r=RaduBerinde a=RaduBerinde

This is a very limited fix for #34810.

The core problem is that we don't take into account that if we have an
ON condition, not only there's a cost to evaluate it on each row, but
we are generating more internal rows to get a given number of output
rows.

I attempted to do a more general fix (for all join types), where I
tried to estimate the "internal" number of rows using
`unknownFilterSelectivity` for each ON condition. There were two
problems:
 - in some cases (especially with lookup joins) we have an extra ON
   condition that doesn't actually do anything:
     `ab JOIN xy ON a=x AND a=10` becomes
     `ab JOIN xy ON a=x AND a=10 AND x=10` becomes
   and `a=10` could remain as an ON condition. This results in bad
   query plans in important cases (e.g. TPCC) where it prefers to do
   an extra lookup join (due to a non-covering index) just because of
   that condition.

 - we don't have the equality columns readily available for hash join
   (and didn't want to extract them each time we cost). In the future
   we may split the planning into a logical and physical stage, and we
   should then separate the logical joins from hash join.

For 19.1, we simply simply add a cost for lookup joins that is
proportional to the number of remaining ON conditions. This is the
least disruptive method that still fixes the case observed in #34810.
I will leave the issue open to address this properly in the next
release.

Note that although hash joins and merge joins have the same issue in
principle, in practice we always generate these expressions with
equality on all possible columns.

Release note: None

35630: storage/tscache: Pick up andy-kimball/arenaskl fix r=nvanbenschoten a=nvanbenschoten

Fixes #31624.
Fixes #35557.

This commit picks up andy-kimball/arenaskl#4.

I strongly suspect that the uint32 overflow fixed in that PR was the
cause of the two index out of bounds panics. See that commit for more
details.

The PR also fixes a bug in memory recylcling within the tscache. I confirmed
on adriatic that over 900 64MB arenas had been allocated since it was last
wiped.

35644: opt: use correct ordering for insert input in execbuilder r=RaduBerinde a=RaduBerinde

We were setting up a projection on the Insert's input but we were
accidentally using the parent Insert's ordering instead of that of the
input.

Fixes #35564.

Release note (bug fix): Fixed a "column not in input" crash when
`INSERT ... RETURNING` is used inside a clause that requires an
ordering.

35651: jobs, sql, ui: Create `AutoCreateStats` job type r=celiala a=celiala

With #34279, enabling the cluster setting
`sql.stats.experimental_automatic_collection.enabled` has the potential
to create many CreateStats jobs, which can cause the Jobs view on the
AdminUI to become cluttered.

This commit creates a new `AutoCreateStats` job type for these auto-created
CreateStats jobs, so that users are able to still see their own manual runs
of CREATE STATISTICS, via the pre-existing `CreateStats` type.

cc @danhhz, @piyush-singh, @rolandcrosby 

![jobs-auto-create-stats](https://user-images.githubusercontent.com/3051672/54212467-5cea2c80-44b9-11e9-9c11-db749814f019.gif)

Release note (admin ui change): AutoCreateStats type added to
Jobs page to filter automatic statistics jobs.

Fixes #34377.

Co-authored-by: Rebecca Taft <becca@cockroachlabs.com>
Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Celia La <celia@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Mar 12, 2019

Build succeeded

@craig craig bot merged commit ceba034 into cockroachdb:master Mar 12, 2019
@RaduBerinde RaduBerinde deleted the lookup-join-cost-fix-lame branch March 15, 2019 12:48
@RaduBerinde RaduBerinde mentioned this pull request Jun 26, 2019
Closed
@RaduBerinde RaduBerinde mentioned this pull request Jul 21, 2019
Merged
craig bot pushed a commit that referenced this pull request Jul 22, 2019
@RaduBerinde
Merge #39016
e2e2c67 
39016: opt: improve per-ON condition cost adjustment for lookup join r=RaduBerinde a=RaduBerinde

Issue #34810 tracks taking into account the internal row count of
lookup joins. We currently have a hack in place to always prefer
looking indexes that constrain more columns (see #35587). We encountered a case
where this adjustment doesn't work: when the estimated row count is
very very small (which happens when there are a lot of conditions),
the per-row cost adjustment ends up not making a difference (this is
because of limited floating point precision, and the "tolerance" built
into `Cost.Less()`). To address this, we also add a constant per-ON
condition cost which isn't scaled by the row count.

Release note (bug fix): Fixed bug in the optimizer causing a bad index
for lookup join in some cases.

Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
@RaduBerinde RaduBerinde mentioned this pull request Jul 22, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinj justinj justinj left review comments

@andy-kimball andy-kimball andy-kimball approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RaduBerinde @cockroach-teamcity @danhhz @justinj @andy-kimball