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

sql,opt: add cluster and session settings for locality optimized search #60771

Merged
merged 1 commit into from
Feb 20, 2021

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Feb 19, 2021

This commit adds a new session setting locality_optimized_partitioned_index_scan
and corresponding cluster default setting
sql.defaults.locality_optimized_partitioned_index_scan.enabled. In future
commits, these settings will be used to enable or disable locality optimized
search. If enabled, the optimizer will try to plan scans and lookup joins in
which local nodes are searched for matching rows before remote nodes,
in the hope that the execution engine can avoid visiting remote nodes.

Informs #55185

Release note (sql change): Added a new session setting
locality_optimized_partitioned_index_scan and corresponding cluster default
setting sql.defaults.locality_optimized_partitioned_index_scan.enabled.
Both are currently disabled by default, and are currently unused. In the
future, these settings will be used to enable or disable locality
optimized search. If enabled, the optimizer will try to search locally
for rows in REGIONAL BY ROW tables before searching remote nodes.

@rytaft rytaft requested review from mgartner, RaduBerinde and a team February 19, 2021 03:33
@rytaft rytaft requested a review from a team as a code owner February 19, 2021 03:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rytaft rytaft force-pushed the locality-opt branch 4 times, most recently from 1158740 to 8369cda Compare February 19, 2021 13:51
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:

[nit] locality_optimized_search sounds very general (e.g. one could reasonably assume it encompasses the locality-sensitive index selection). Maybe locality_optimized_partitioned_index_scan?

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


pkg/sql/exec_util.go, line 296 at r1 (raw file):

var localityOptimizedSearchMode = settings.RegisterBoolSetting(
	"sql.defaults.locality_optimized_search.enabled",
	"default value for locality_optimized_search session setting;"+

[nit] "disables locality optimizer search by default" is confusing (sounds like that's what happens if we set it to true). I'd just leave it out.

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!

[nit] locality_optimized_search sounds very general (e.g. one could reasonably assume it encompasses the locality-sensitive index selection). Maybe locality_optimized_partitioned_index_scan?

Done.

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


pkg/sql/exec_util.go, line 296 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] "disables locality optimizer search by default" is confusing (sounds like that's what happens if we set it to true). I'd just leave it out.

Done. I also added a bit more info about what it actually does (for documentation purposes).

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! 1 of 0 LGTMs obtained (waiting on @mgartner)

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.

Thanks!

bors r+

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

@rytaft
Copy link
Collaborator Author

rytaft commented Feb 19, 2021

bors r-

@craig
Copy link
Contributor

craig bot commented Feb 19, 2021

Canceled.

This commit adds a new session setting
locality_optimized_partitioned_index_scan and corresponding cluster default
setting sql.defaults.locality_optimized_partitioned_index_scan.enabled.
In future commits, these settings will be used to enable or disable
locality optimized search. If enabled, the optimizer will try to plan
scans and lookup joins in which local nodes are searched for matching
rows before remote nodes, in the hope that the execution engine can
avoid visiting remote nodes.

Informs cockroachdb#55185

Release note (sql change): Added a new session setting
locality_optimized_partitioned_index_scan and corresponding cluster default
setting sql.defaults.locality_optimized_partitioned_index_scan.enabled.
Both are currently disabled by default, and are currently unused. In the
future, these settings will be used to enable or disable locality
optimized search. If enabled, the optimizer will try to search locally
for rows in REGIONAL BY ROW tables before searching remote nodes.
@rytaft
Copy link
Collaborator Author

rytaft commented Feb 19, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 20, 2021

Build failed (retrying...):

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 5 of 10 files at r1, 5 of 5 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)

@craig
Copy link
Contributor

craig bot commented Feb 20, 2021

Build succeeded:

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.

4 participants