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: prefer order-matching index if there is a limit #5446

Merged
merged 2 commits into from
Mar 22, 2016

Conversation

RaduBerinde
Copy link
Member

In #4925, we observed ineffective planning for a query in the photos app. We
prefer to use the primary index and sort rather than use a non-covering index
which makes sense in general (non-covering indices require and expensive
indexJoin) but in this case we also had a limit. In such a case using the index
would require looking only at the first rows instead of getting all matching
rows and sorting.

In this change we tweak the index selection: if we have a reasonable limit, we
give a "boost" to all indices that match the ordering exactly. The boost exactly
offsets the non-covering index penalty.

In addition to the new tests, I also verified the photo app query in #4925 now
uses the index.

Fixes #5246.


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 22, 2016

commit message: s/and expensive/an expensive/


Reviewed 2 of 2 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 6 unresolved discussions.


sql/select.go, line 313 [r1] (raw file):
this comment seems to have lost the code it once referred to. (that happened in the first commit).


sql/select.go, line 308 [r2] (raw file):
expand this comment to explain why "if not grouping". I guess it's because grouping implies an unbounded number of rows need to be scanned.


sql/select.go, line 311 [r2] (raw file):
this doesn't seem like sufficient overflow protection - limitCount may not be equal to MaxInt64, but the sum of limitCount and limitOffset may still exceed it and wrap around. Perhaps limitCount+limitOffset >=0 && limitCount+limitOffset <= 1000 instead?


sql/select.go, line 670 [r2] (raw file):
analyzeOrdering also needs this comment, yes?


sql/select.go, line 936 [r2] (raw file):
the commit message says this directly offsets some other cost - can you use a constant so that the relationship is obvious? also a comment right on this block would be good.


sql/testdata/select_non_covering_index, line 89 [r2] (raw file):
expand this comment to explain that the primary index is still consulted because the order-matching index is non-covering.


Comments from the review on Reviewable.io

@RaduBerinde
Copy link
Member Author

TFTR! Updated.


Review status: 3 of 5 files reviewed at latest revision, 6 unresolved discussions.


sql/testdata/select_non_covering_index, line 89 [r2] (raw file):
This is the theme of the entire file (select_non_covering_index)


Comments from the review on Reviewable.io

@tamird
Copy link
Contributor

tamird commented Mar 22, 2016

Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


sql/limit.go, line 69 [r3] (raw file):
what's this about? is there a test that induces this case?


sql/testdata/select_non_covering_index, line 89 [r2] (raw file):
Ah, and the other test cases all touch the primary as well. Fair enough.


Comments from the review on Reviewable.io

@RaduBerinde
Copy link
Member Author

Review status: 5 of 6 files reviewed at latest revision, 1 unresolved discussion.


sql/limit.go, line 69 [r3] (raw file):
Added tests.


Comments from the review on Reviewable.io

In cockroachdb#4925, we observed ineffective planning for a query in the photos app. We
prefer to use the primary index and sort rather than use a non-covering index
which makes sense in general (non-covering indices require an expensive
indexJoin) but in this case we also had a limit. In such a case using the index
would require looking only at the first rows instead of getting all matching
rows and sorting.

In this change we tweak the index selection: if we have a reasonable limit, we
give a "boost" to all indices that match the ordering exactly. The boost exactly
offsets the non-covering index penalty.

In addition to the new tests, I also verified the photo app query in cockroachdb#4925 now
uses the index.

Fixes cockroachdb#5246.
@tamird
Copy link
Contributor

tamird commented Mar 22, 2016

:lgtm:


Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

RaduBerinde added a commit that referenced this pull request Mar 22, 2016
sql: prefer order-matching index if there is a limit
@RaduBerinde RaduBerinde merged commit 0f7642c into cockroachdb:master Mar 22, 2016
@RaduBerinde RaduBerinde deleted the index-sel-limit branch March 22, 2016 17:39
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.

2 participants