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: add optimizer_min_row_count session setting #140065

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

mgartner
Copy link
Collaborator

Informs #64570
Informs #130201

Release note (sql change): The optimizer_min_row_count session setting
has been added which sets a lower bound on row count estimates for
relational expressions during query planning. A value of zero, which is
the default, indicates no lower bound. Note that if this is set to a
value greater than zero, a row count of zero can still be estimated for
expressions with a cardinality of zero, e.g., for a contradictory
filter. Setting this to a value higher than 0, such as 1, may yield
better query plans in some cases, such as when statistics are frequently
stale and inaccurate.

@mgartner mgartner added backport-23.2.x Flags PRs that need to be backported to 23.2. backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.2.x backport-24.3.x Flags PRs that need to be backported to 24.3 backport-25.1.x Flags PRs that need to be backported to 25.1 labels Jan 29, 2025
@mgartner mgartner requested a review from a team January 29, 2025 20:58
@mgartner mgartner requested a review from a team as a code owner January 29, 2025 20:58
@mgartner mgartner requested review from michae2 and removed request for a team January 29, 2025 20:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@mgartner

This comment was marked as outdated.

Informs cockroachdb#64570
Informs cockroachdb#130201

Release note (sql change): The `optimizer_min_row_count` session setting
has been added which sets a lower bound on row count estimates for
relational expressions during query planning. A value of zero, which is
the default, indicates no lower bound. Note that if this is set to a
value greater than zero, a row count of zero can still be estimated for
expressions with a cardinality of zero, e.g., for a contradictory
filter. Setting this to a value higher than 0, such as 1, may yield
better query plans in some cases, such as when statistics are frequently
stale and inaccurate.
Copy link
Collaborator

@michae2 michae2 left a comment

Choose a reason for hiding this comment

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

:lgtm: Great idea to make it configurable. This is nice!

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

Copy link
Contributor

@mw5h mw5h 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 25 of 25 files at r1, all commit messages.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball, @mgartner, and @rytaft)


pkg/sql/opt/memo/statistics_builder_test.go line 115 at r1 (raw file):

		relProps.NotNullCols = cs.ExtractNotNullCols(ctx, &evalCtx)
		s := relProps.Statistics()
		const minRowCount = 0

Worth re-running this test with minRowCount = 1?


pkg/sql/opt/memo/statistics_builder.go line 842 at r1 (raw file):

func (sb *statisticsBuilder) buildScan(scan *ScanExpr, relProps *props.Relational) {
	s := relProps.Statistics()
	if zeroCardinality := s.Init(relProps, sb.minRowCount); zeroCardinality {

Boilerplate like this is great opportunity to refactor. If the Go designers thought highly enough of us to think we could use macros without hurting ourselves, this would be an excellent use, but we'll have to make do with passing function pointers around.

I know you're on deadline, so fine if you don't want to deal with this now.

Copy link
Collaborator Author

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

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


pkg/sql/opt/memo/statistics_builder_test.go line 115 at r1 (raw file):

Previously, mw5h (Matt White) wrote…

Worth re-running this test with minRowCount = 1?

The coverage of this test is minimal, so I don't think we get much from this. I think the datadriven tests I added are sufficient.

Copy link
Collaborator Author

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

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


pkg/sql/opt/memo/statistics_builder.go line 842 at r1 (raw file):

Previously, mw5h (Matt White) wrote…

Boilerplate like this is great opportunity to refactor. If the Go designers thought highly enough of us to think we could use macros without hurting ourselves, this would be an excellent use, but we'll have to make do with passing function pointers around.

I know you're on deadline, so fine if you don't want to deal with this now.

Talked in Slack - we'll keep as-is for now, but we can also consider refactoring this in the future!

@mgartner
Copy link
Collaborator Author

TFTRs!

bors r+

@craig craig bot merged commit 281a25b into cockroachdb:master Jan 31, 2025
22 checks passed
Copy link

blathers-crl bot commented Jan 31, 2025

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 da6fbb5 to blathers/backport-release-24.1-140065: 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 24.1.x failed. See errors above.


error creating merge commit from da6fbb5 to blathers/backport-release-24.2-140065: 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 24.2.x failed. See errors above.


error creating merge commit from da6fbb5 to blathers/backport-release-24.3-140065: 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 24.3.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-24.1.x Flags PRs that need to be backported to 24.1. backport-24.3.x Flags PRs that need to be backported to 24.3 backport-25.1.x Flags PRs that need to be backported to 25.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants