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

Annotate ActiveRecord queries not using index #31760

Merged
merged 2 commits into from
Nov 25, 2020
Merged

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Nov 7, 2019

Description

This PR adds a red [NO INDEX] annotation to ActiveRecord SQL notifications (printed in Rails development console by default) for queries where the MySQL query result has the no_index_used flag set:

Before:
image

After:
image

This should provide some lightweight DB-optimization guidance for development and debugging.

It also adds a raise_on_no_index_used Rails configuration option, which will enforce index usage by raising NoIndexUsedException if any ActiveRecord query is executed without using an index. This option is disabled by default, since MySQL often chooses to ignore an existing index for small or empty tables where the query optimizer decides that a table scan will be more efficient anyway. (I thought I would include the option anyway, just in case it may be eventually useful.)

Background

The MySQL client/server protocol distinguishes queries that make use of an index for efficient lookup, and those that do not use an index (sometimes called 'full table scan' queries). This information is passed in the Status Flags of the OK packet produced with every MySQL-protocol query response, and is set to the :no_index_used key of the Mysql2::Result#server_flags object produced by the mysql2 gem.

Implementation

This PR adds a Rails initializer that patches the ActiveRecord::ConnectionAdapters::Mysql2Adapter to add text to the name-key string on the sql.active_record notification generated for every query, when the no_index_used flag on the Result object is set.

Future work

  • Filter out benign cases of full-table scans so we can enforce index usage throughout the rest of the application.
  • Use the SUM_NO_INDEX_USED column of the performance_schema.events_statements_summary_by_digest Performance Schema summary table to monitor no-index-usage in production.

Testing story

Manual testing and the existing test suite which could verify that nothing will break as a result of this change.

Tests covering the actual behavior in this PR are deferred fro now- this PR was done quickly to help address an existing pain point in development, and creating a reliable test-case with a no-index query might not be trivial.

@wjordan
Copy link
Contributor Author

wjordan commented Feb 21, 2020

Before merging I want to update this to always be skipped in production env, in case the added instrumentation triggers any bugs or issues.

@uponthesun
Copy link

What about something like notifications in slack when we do queries without indexes during DTT? And / or enabling the option to raise NoIndexUsedException on DTTs or drone runs?

@wjordan
Copy link
Contributor Author

wjordan commented Feb 25, 2020

What about something like notifications in slack when we do queries without indexes during DTT?

Sounds interesting, though probably separate work beyond the scope of this PR.

And / or enabling the option to raise NoIndexUsedException on DTTs or drone runs?

Right now, this would raise an exception in a tremendous amount of normal cases, due to the limitation I noted ('MySQL often chooses to ignore an existing index for small or empty tables where the query optimizer decides that a table scan will be more efficient anyway.') It might be possible with further focused work to ignore all cases where this happens, but that's a lot of extra work best saved for a separate effort.

@Erin007
Copy link
Contributor

Erin007 commented Mar 6, 2020

Yes! I think this potentially would've helped catch issues discussed in the recent flaky test COE; better yet, I know it will be useful as I move into looking more carefully at progress tab performance. There are clearly uses beyond that, but 👍 from me on the idea!

Add Rails config `raise_on_no_index_used` to raise exception.
Disable by default in production environment
@wjordan
Copy link
Contributor Author

wjordan commented Nov 25, 2020

Before merging I want to update this to always be skipped in production env, in case the added instrumentation triggers any bugs or issues.

Finally implemented this part so I can (finally) merge this PR.

@wjordan wjordan merged commit 575a330 into staging Nov 25, 2020
@wjordan wjordan deleted the mysql_check_index_used branch November 25, 2020 00:02
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.

None yet

3 participants