Skip to content

Adding the available_queries_for_explain thread local into the AR Runtime Registry #10198

Closed
wants to merge 3 commits into from

3 participants

@wangjohn

Before, the :available_queries_for_explain thread local could be accessed by using Thread.current[:available_queries_for_explain]. However, the ActiveRecord::RuntimeRegistry object has recently been added to limit the number of thread locals that are passed around.

This PR moves the handling of :available_queries_for_explain into the RuntimeRegistry object and changes the appropriate logic.

@jeremy jeremy and 1 other commented on an outdated diff Apr 13, 2013
activerecord/lib/active_record/runtime_registry.rb
@@ -27,6 +27,11 @@ module ActiveRecord
class RuntimeRegistry
extend ActiveSupport::PerThreadRegistry
- attr_accessor :connection_handler, :sql_runtime, :connection_id
+ attr_accessor :connection_handler, :sql_runtime, :connection_id,
+ :available_queries_for_explain
+
+ def initialize
+ @available_queries_for_explain = []
@jeremy
Ruby on Rails member
jeremy added a note Apr 13, 2013

Looks like nil is the current default. Prefer [] also. Is there a test for this?

@fxn
Ruby on Rails member
fxn added a note Apr 13, 2013

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jeremy jeremy commented on an outdated diff Apr 13, 2013
activerecord/test/cases/explain_subscriber_test.rb
yield queries
ensure
- Thread.current[:available_queries_for_explain] = nil
+ ActiveRecord::RuntimeRegistry.instance.available_queries_for_explain = nil
@jeremy
Ruby on Rails member
jeremy added a note Apr 13, 2013

Saving & restoring the original value would be cleaner and avoid needing knowledge of the default value.

@jeremy
Ruby on Rails member
jeremy added a note Apr 13, 2013

Since collecting_queries_for_explain does the same dance, could consider adding a with_queries_for_explain to RuntimeRegistry. However, that kind of behavior feels out of scope for a mere registry...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@wangjohn

@jeremy Thanks for the comments. I've added a bunch of functionality to the RuntimeRegistry, though it might be overkill for the save and restore thing that was going on with available_queries_for_explain. I've also added a tests for the registry.

@fxn fxn commented on an outdated diff Apr 13, 2013
activerecord/lib/active_record/explain.rb
@@ -20,7 +19,7 @@ def exec_explain(queries) # :nodoc:
[].tap do |msg|
msg << "EXPLAIN for: #{sql}"
unless bind.empty?
- bind_msg = bind.map {|col, val| [col.name, val]}.inspect
+ bind_msg = bind.map { |col, val| [col.name, val] }.inspect
@fxn
Ruby on Rails member
fxn added a note Apr 13, 2013

This is a personal preference edit, not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fxn fxn commented on an outdated diff Apr 13, 2013
activerecord/lib/active_record/explain_subscriber.rb
@@ -7,7 +7,7 @@ def start(name, id, payload)
end
def finish(name, id, payload)
- if queries = Thread.current[:available_queries_for_explain]
+ if queries = ActiveRecord::RuntimeRegistry.instance.available_queries_for_explain
@fxn
Ruby on Rails member
fxn added a note Apr 13, 2013

Shouldn't this check now for empty?? Also in exec_explain in explain.rb?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@fxn fxn commented on an outdated diff Apr 13, 2013
activerecord/lib/active_record/explain.rb
yield
- return current[:available_queries_for_explain]
+ return ActiveRecord::RuntimeRegistry.instance.available_queries_for_explain
@fxn
Ruby on Rails member
fxn added a note Apr 13, 2013

Registries have class-level accessors:

ActiveRecord::RuntimeRegistry.available_queries_for_explain

I believe either we should use them, or remove the method_missing callback.

@fxn
Ruby on Rails member
fxn added a note Apr 13, 2013

Addressed in 11c6973.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@wangjohn

@fxn If we change the explain_subscriber.rb file (https://github.com/rails/rails/blob/master/activerecord/lib/active_record/explain_subscriber.rb#L9-L13) to check for whether queries.empty?, then we will break the following test: https://github.com/rails/rails/blob/master/activerecord/test/cases/explain_subscriber_test.rb#L7-L12. You wrote this, so you probably have better insight as to whether this is desired functionality or not.

However, if we had available_queries_for_explain = [], then whenever you called the finish function, you would never add anything to the list of available_queries_for_explain. I don't think that is desired behavior, and I believe that checking for nil in that file was just trying to assure that queries was an array, though I'm not sure.

@fxn fxn added a commit that referenced this pull request Apr 13, 2013
@fxn fxn hides the per thread registry instance, and caches singleton methods
Existing code was delegating to the instance with delegate
macro calls, or invoking the instance method to reach
the object and call its instance methods.

But the point is to have a clean class-level interface where
the thread local instance is hidden in the implementation.

References #11c6973.
References #10198.
e5ef3ab
@fxn
Ruby on Rails member
fxn commented Apr 13, 2013

You are right.

In the current implementation, the thread local is only checked on demand, and if a explain is going on this code sets the collection to fill:

https://github.com/rails/rails/blob/master/activerecord/lib/active_record/explain.rb#L8

The finish method of the subscriber uses the thread local also as a flag that indicates whether a EXPLAIN is going on. If we are collecting queries the collection should be filled, otherwise there are no queries to collect, do nothing.

But with your refactor we have a canonical place to initialize this with a default, and I prefer anything that is plural to return a collection, an empty array is a better default in my view.

Then, the initializer could set an actual flag, and the subscriber could fill the collection if the flag is active.

Granted, the default empty array is never used, but I find that setup to be more intuitive. I believe @jeremy sees it the same way.

wangjohn added some commits Apr 12, 2013
@wangjohn wangjohn Adding the available_queries_for_explain thread local into the
ActiveRecord runtime registry to try to manage thread locals.
3b8ca63
@wangjohn wangjohn Added tests for the runtime registry making sure that the correct save
and restore behavior occurs, as well as the correct behavior regarding
arrays for the available_queries_for_explain attribute.
5fef486
@wangjohn wangjohn Adding a means for saving available queries on the runtime registry.
This sets the collecting_queries_flag to true, and allows the finish
method to run correctly in the explain_subscriber.
4efabe5
@wangjohn

@fxn Ok, thanks for the clarification. I've added a flag to detect whether an EXPLAIN is happening or not, which is now included in the runtime registry. Now the finish method checks to see if collecting_queries_flag is true or not, before it appends the parse payload.

Now, calling save_available_queries will set the collecting_queries_flag to true, and the restore_available_queries method will set the flag to false.

@fxn
Ruby on Rails member
fxn commented Apr 15, 2013

I feel the patch is too complicated. You could just assign the empty array to ActiveRecord::RuntimeRegistry.available_queries_for_explain as the previous code did couldn't you?

Also "_flag" is not a good name, ActiveRecord::RuntimeRegistry.collect_queries? would maybe be better.

@fxn fxn closed this in ef7a48d Apr 16, 2013
@wangjohn wangjohn deleted the wangjohn:explain_thread_locals_are_begin_registered branch May 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.