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

[9.x] Add index hinting support to query builder #46063

Merged
merged 2 commits into from
Feb 10, 2023

Conversation

cwhite92
Copy link

@cwhite92 cwhite92 commented Feb 9, 2023

This PR adds useIndex(), forceIndex() and ignoreIndex() methods to the query builder, to help MySQL's (and others) query planner use the indexes I want for a query. I've had to do this in the past to optimize slow queries where the planner didn't choose to use an index even though it was dramatically more performant, and today I came across @assertchris doing the same thing, so I thought it might be worth adding to the ORM for other people to use.

Support across engines is spotty, with only MySQL supporting all three hinting modes. Based on my research this seems to be the support:

Database Index hinting Index forcing Index ignoring
MySQL ✅ Supported ✅ Supported ✅ Supported
SQLite ❌ Not supported ✅ Supported ❌ Not supported (can only ignore all indexes)
PostgreSQL ❌ Not supported ❌ Not supported ❌ Not supported
SQL Server ❌ Not supported ✅ Supported ❌ Not supported (can only ignore all indexes)

I have only implemented these where supported, for engines without support the query will be unchanged by the useIndex(), forceIndex() and ignoreIndex() methods.

I have also only added support for hinting one index. MySQL supports the use of multiple USE INDEX hints, and a combination of USE INDEX hints with IGNORE INDEX hints, but I have left that unimplemented for simplicity and to test the water on if this change is wanted. If we want to add support for hinting multiple indexes, I can add that functionality.

A 4th method called ignoreIndexes() could be added to support SQLite and SQL Server's ability to ignore all indexes and do full table scans, but I struggle to think of why you'd actually want to do that in a real environment.

@taylorotwell taylorotwell merged commit 8a9123a into laravel:9.x Feb 10, 2023
@decadence
Copy link
Contributor

decadence commented Feb 11, 2023

Note from MySQL documentation. They have plans to deprecate and remove these hints.

As of MySQL 8.0.20, the server supports the index-level optimizer hints JOIN_INDEX, GROUP_INDEX, ORDER_INDEX, and INDEX, which are equivalent to and intended to supersede FORCE INDEX index hints, as well as the NO_JOIN_INDEX, NO_GROUP_INDEX, NO_ORDER_INDEX, and NO_INDEX optimizer hints, which are equivalent to and intended to supersede IGNORE INDEX index hints. Thus, you should expect USE INDEX, FORCE INDEX, and IGNORE INDEX to be deprecated in a future release of MySQL, and at some time thereafter to be removed altogether.

@taylorotwell
Copy link
Member

Hmm, well, we probably should not have added this then.

@cwhite92
Copy link
Author

cwhite92 commented Feb 15, 2023

Damn, I wasn't aware of that depreciation. This complicates things for MySQL > 8.0.

We can still support forcing and ignoring indexes, but the syntax and where you specify the hinting in the query has now changed to:

select /*+ index(users users_email_unique) */ email
from users
where email = 'user@domain.com';

select /*+ no_index(users users_email_unique) */ email
from users
where email = 'user@domain.com';

(I attempted to move the index hinting to different parts of the query, but it only works if it's between the select and list of columns)

So to support all versions of MySQL without introducing a breaking change, I'd have to:

  1. Add compileAggregate() and compileColumns() to the MySqlGrammar class to insert the index hint there for MySQL > 8.0; and
  2. Keep the current compileIndexHint() method in MySqlGrammar for MySQL < 8.0; and
  3. Check for the MySQL version in all of those three methods, which I don't seem to currently have access to inside the grammar classes.

I could simplify this and only support index hinting on MySQL 8.0 and above, but that would be a breaking change since this PR is already released with Laravel 10, so I assume it'd go to master for Laravel 11?

Let me know how you'd like to handle this.

@decadence
Copy link
Contributor

@cwhite92 What decision have you made about it? Because current MySQL implementation will stop working at one point I think.

I could simplify this and only support index hinting on MySQL 8.0 and above, but that would be a breaking change since this PR is already released with Laravel 10, so I assume it'd go to master for Laravel 11?

I think this is the most suitable for this situation.

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