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: adjust cost of scan with unbounded cardinality to avoid bad plans #68676

Merged
merged 1 commit into from
Aug 11, 2021

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Aug 10, 2021

This commit tweaks the application of the unbounded cardinality penalty
in the coster to add it directly to the cost of scans and zigzag joins
rather than to the row count. This helps prevent an issue in which the
number of index columns could dominate the scan cost and result in
suboptimal plans.

Fixes #68556

Release note (bug fix): Fixed a regression in the optimizer's cost model
that could cause it to choose suboptimal plans when choosing between two
non-unique index scans with different numbers of columns per index.

@rytaft rytaft requested review from mgartner, michae2 and a team August 10, 2021 22:30
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Nice! :lgtm:

Reviewed 40 of 40 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2 and @rytaft)


pkg/sql/opt/xform/coster.go, line 663 at r1 (raw file):

	// number of index columns does not have an outsized effect on the cost of
	// the scan. See issue #68556.
	baseCost += memo.Cost(c.largeCardinalityCostPenalty(scan.Relational().Cardinality, rowCount))

nit: if c.LargeCardinalityCostPenalty() returns a memo.Cost instead of a float64, this line is simplified.


pkg/sql/opt/xform/coster.go, line 1069 at r1 (raw file):

	// the number of index columns does not have an outsized effect on the cost of
	// the zigzag join. See issue #68556.
	cost += memo.Cost(c.largeCardinalityCostPenalty(join.Relational().Cardinality, rowCount))

ditto

Copy link
Collaborator Author

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

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @michae2)


pkg/sql/opt/xform/coster.go, line 663 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: if c.LargeCardinalityCostPenalty() returns a memo.Cost instead of a float64, this line is simplified.

Done.


pkg/sql/opt/xform/coster.go, line 1069 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

ditto

Done.

Copy link
Collaborator

@mgartner mgartner left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @michae2)

@rytaft
Copy link
Collaborator Author

rytaft commented Aug 11, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 11, 2021

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 11, 2021

Build failed (retrying...):

craig bot pushed a commit that referenced this pull request Aug 11, 2021
68676: opt: adjust cost of scan with unbounded cardinality to avoid bad plans r=rytaft a=rytaft

This commit tweaks the application of the unbounded cardinality penalty
in the coster to add it directly to the cost of scans and zigzag joins
rather than to the row count. This helps prevent an issue in which the
number of index columns could dominate the scan cost and result in
suboptimal plans.

Fixes #68556

Release note (bug fix): Fixed a regression in the optimizer's cost model
that could cause it to choose suboptimal plans when choosing between two
non-unique index scans with different numbers of columns per index.

Co-authored-by: Rebecca Taft <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 11, 2021

Build failed:

This commit tweaks the application of the unbounded cardinality penalty
in the coster to add it directly to the cost of scans and zigzag joins
rather than to the row count. This helps prevent an issue in which the
number of index columns could dominate the scan cost and result in
suboptimal plans.

Fixes cockroachdb#68556

Release note (bug fix): Fixed a regression in the optimizer's cost model
that could cause it to choose suboptimal plans when choosing between two
non-unique index scans with different numbers of columns per index.
@rytaft
Copy link
Collaborator Author

rytaft commented Aug 11, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 11, 2021

Build succeeded:

@craig craig bot merged commit 348be0b into cockroachdb:master Aug 11, 2021
@blathers-crl
Copy link

blathers-crl bot commented Aug 11, 2021

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 9a3d1ab to blathers/backport-release-21.1-68676: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

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: unbounded max cardinality penalty makes number of index columns dominate scan cost
3 participants