Add `config.active_record.warn_on_records_fetched_greater_than` option #18846

hundredwatt · 2015-02-07T20:34:05Z

When set to an integer, a warning will be logged whenever a result set
larger than the specified size is returned by a query. Fixes #16463

kaspth · 2015-02-07T20:46:33Z

activerecord/lib/active_record/core.rb

+      mattr_accessor :warn_on_result_set_size, instance_writer: false
+      self.warn_on_result_set_size = nil
+
+


✂️ this line

hundredwatt · 2015-02-07T20:52:16Z

@kaspth ✂️-ed

hundredwatt · 2015-02-08T16:55:07Z

Wondering if we should move the exec_queries method to a module and then use inheritance to add the warning only if the config value is set. That way, this would have 0 cost when not enabled.

kaspth · 2015-02-08T17:37:38Z

I don't think you need to touch the existing #exec_queries method. You could try the module approach using #prepend.

Unless #prepend is frowned upon in Rails, which I need someone else to verify.

hundredwatt · 2015-02-08T18:05:33Z

I had the same thought, however there's no other use of Module#prepend in AR. I believe the idea was to start allowing it for Rails 5, when backwards compatibility for 1.9 will be dropped.

hundredwatt · 2015-02-08T18:24:24Z

Gave Module#prepend a try with c5751fe. This will fail CI, but I'm curious to get some feedback on the approach.

hundredwatt · 2015-02-08T18:27:50Z

Nevermind comment about CI, travis-ci matrix is only Ruby 2.2 now.

kaspth · 2015-02-08T18:31:02Z

activerecord/test/cases/relation/warn_on_result_set_size_test.rb

+require 'models/post'
+require 'models/comment'
+require 'models/author'
+require 'models/rating'


Can we remove the requires for the other models?

kaspth · 2015-02-08T18:35:26Z

I think you need to rebase this on master before Travis will try a build.

kaspth · 2015-02-08T18:36:30Z

Otherwise the code looks good to me 👍. It depends on whether others are okay with the initializer approach here.

hundredwatt · 2015-02-08T18:38:55Z

Thanks for feedback @kaspth 😄. Will await additional feedback and then can squash before merging.

kaspth · 2015-02-08T18:49:22Z

Sure thing 😄

Your test leaks state. The module you prepend is still in Relation after the test, so all the other tests fail because you're comparing x >= nil where x is an integer.

I don't know if unprepending a module is considered a dirty practice, so let's wait for others to chime in 😁

kaspth · 2015-02-08T18:51:46Z

What you did just now is probably fine too :)

hundredwatt · 2015-02-08T18:51:58Z

@kaspth 👍

hundredwatt · 2015-02-08T20:44:38Z

Just patched Delayed Job in a production Rails app with this warning and set up a log alert. Already found an instance of Model.each that should be Model.find_each 😄:

Feb 08 15:00:44 GaggleAMP: Result set size exceeded 1001 (model: Authentication::GoogleAuthentication, records: 1507)

kaspth · 2015-02-08T21:16:59Z

Cool! I think the error message needs a little tweaking to read a bit more naturally.

Would it be of use to broadcast this as an Active Support notification too?

rafaelfranca · 2015-02-10T19:21:04Z

activerecord/lib/active_record/railtie.rb

@@ -102,6 +102,17 @@ class Railtie < Rails::Railtie # :nodoc:
      end
    end

+    initializer "active_record.warn_on_result_set_size" do
+      if config.active_record.warn_on_result_set_size
+        config.after_initialize do |app|


Why we need the after_initialize block?

It's not needed, its only there because I copied and pasted from the previous initializer, doh! Will remove

kaspth · 2015-02-10T22:09:29Z

I've been thinking about the config option name and result_set doesn't fit with Active Record's terminology.
What about something like warn_on_fetches_above or warn_on_record_fetches_greater_than?

kaspth · 2015-02-10T22:18:07Z

activerecord/lib/active_record/relation/warn_on_result_set_size.rb

+        super.tap do
+          if logger && warn_on_result_set_size
+            if @records.length >= warn_on_result_set_size
+              logger.warn "Result set size exceeded #{warn_on_result_set_size} (model: #{@klass}, records: #{@records.size})"


What about changing the message to something like:

"Large query detected. Exceeded #{warn_on_result_set_size} threshold by loading #{@records.size} #{@klass} records."

hundredwatt · 2015-02-11T19:55:58Z

@kaspth I like where you're going. How about:

For the config setting: config.active_record.warn_on_records_fetched_greater_than

For the message:

"Large query detected. Exceeded #{warn_on_result_set_size} threshold by fetching #{@records.size} #{@klass} records."

For the module, maybe we can call it LargeQueryDetection instead of WarnOnResultSetSize?

kaspth · 2015-02-12T16:59:17Z

I like your suggestions way better than warn_on_result_set_size 😄

I have some more thoughts about the warning message.

There's a part of me that feels fetching is to lightweight a word. Is there a word that better fits we're dealing with a large query?

I think we should explain the threshold just a bit more. Something like this reads a bit broken:
"Large query detected. Exceeded 1001 threshold by fetching 1200 Kitten records."

hundredwatt · 2015-02-13T19:56:44Z

Agreed, I'm struggling with how to phrase this 😄. Here's a number of ideas:

"Query returned large result set. Warning threshold of #{warn_on_result_set_size} exceeded by fetching #{@records.size} #{@klass} records."

"Query fetched #{@records.size} #{@klass} records, exceeding threshold of {warn_on_result_set_size}."

"Query fetched large number of records:  #{@records.size} #{@klass} records (exceeded threshold of #{warn_on_result_set_size})."

Alternatively, do we need to re-state the configured value by including #{warn_on_result_set_size}? Since its explicitly set I'm not sure we need to repeat in the warning message it:

"Query fetched #{@records.size} #{@klass} records"

or maybe have a recommendation?:

"Query fetched #{@records.size} #{@klass} records. Please use batch finder methods to limit memory consumption."

Do any of those options resonate with you?

hundredwatt · 2015-02-13T20:00:41Z

Additionally, we could probably use AS::Notifications to capture the SQL query itself and include it in the warning. I'll play with that.

kaspth · 2015-02-14T21:54:58Z

Well done and thanks for the kind words 👍

I'm going to need someone else to look through the notification stuff, so feel free to jump in.

hundredwatt · 2015-02-26T14:46:27Z

I squashed my previous commits and fixed the documentation as per @fxn's recommendation

kaspth · 2015-02-26T19:22:45Z

activerecord/lib/active_record/relation/record_fetch_warning.rb

+      # When this module is prepended to ActiveRecord::Relation and
+      # `config.active_record.warn_on_records_fetched_greater_than` is
+      # set to an integer, if the number of records a query returns is
+      # greater than the value of `warn_on_records_fetched_greater_than`,


Your documentation says greater than but the check is >= 😉

kaspth · 2015-03-25T16:44:12Z

@hundredwatt hey, completely forgot about this until today 😄

Can you rebase and squash your commits? Then this gets my 👍

Great work, ❤️

When set to an integer, a warning will be logged whenever a result set larger than the specified size is returned by a query. Fixes rails#16463 The warning is outputed a module which is prepended in an initializer, so there will be no performance impact if `config.active_record.warn_on_records_fetched_greater_than` is not set.

hundredwatt · 2015-03-25T20:51:31Z

@kaspth No worries, I know there's a lot of other PRs out there 😉

I rebased/squashed, but the Travis build didn't kick off. Any ideas?

hundredwatt · 2015-03-25T20:52:57Z

@kaspth Nevermind, its building now

kaspth · 2015-03-25T21:11:13Z

activerecord/lib/active_record/relation/record_fetch_warning.rb

+        payload = args.last
+
+        QueryRegistry.queries << payload[:sql]
+      end


Could accomplish the same thing with:

ActiveSupport::Notifications.subscribe("sql.active_record") do |*, payload| QueryRegistry.queries << payload[:sql] end

kaspth · 2015-03-25T21:12:43Z

The build passed, but I don't know why GitHub didn't signal anything here.

hundredwatt · 2015-03-25T21:17:01Z

The build passed, but I don't know why GitHub didn't signal anything here.

¯_(ツ)_/¯

Add `config.active_record.warn_on_result_set_size` option

kaspth · 2015-03-26T20:29:55Z

@hundredwatt @rafaelfranca ❤️

Add `config.active_record.warn_on_records_fetched_greater_than` to the Configuring Rails Guide.

[skip ci] Update configuring.md with #18846

hundredwatt · 2015-03-27T12:20:02Z

@kaspth @rafaelfranca Thanks so much for your feedback! 😄 💙

aripollak · 2015-04-06T15:19:38Z

activerecord/lib/active_record/relation/record_fetch_warning.rb

+      # `config.active_record.warn_on_records_fetched_greater_than` is
+      # set to an integer, if the number of records a query returns is
+      # greater than the value of `warn_on_records_fetched_greater_than`,
+      # a warning is logged. This allows for the dection of queries that


s/dection/detection/

This change was merged a while ago and after looking at the master branch I see it's already fixed: fe6de0f 😁

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 4e52ea1 to 88ba6ec Compare February 7, 2015 20:39

kaspth reviewed Feb 7, 2015
View reviewed changes

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 88ba6ec to 79e8929 Compare February 7, 2015 20:51

kaspth added the activerecord label Feb 7, 2015

kaspth added this to the 5.0.0 milestone Feb 7, 2015

kaspth reviewed Feb 8, 2015
View reviewed changes

hundredwatt force-pushed the feat/warn-on-result-set-size branch from c5751fe to 7bd96e4 Compare February 8, 2015 18:33

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 7bd96e4 to 12e2561 Compare February 8, 2015 18:37

rafaelfranca reviewed Feb 10, 2015
View reviewed changes

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 3e9d3da to 6aaf751 Compare February 10, 2015 19:59

kaspth reviewed Feb 10, 2015
View reviewed changes

hundredwatt force-pushed the feat/warn-on-result-set-size branch from d7d676d to 24d6b9f Compare February 14, 2015 21:11

hundredwatt force-pushed the feat/warn-on-result-set-size branch from ee2af4b to b1b0cb4 Compare February 26, 2015 14:45

kaspth reviewed Feb 26, 2015
View reviewed changes

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 1c6f28f to 3c6675c Compare March 16, 2015 03:06

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 3c6675c to 613abe2 Compare March 25, 2015 20:00

hundredwatt force-pushed the feat/warn-on-result-set-size branch from 613abe2 to 4d6fbe2 Compare March 25, 2015 20:50

kaspth reviewed Mar 25, 2015
View reviewed changes

rafaelfranca added a commit that referenced this pull request Mar 26, 2015

Merge pull request #18846 from hundredwatt/feat/warn-on-result-set-size

8b451e3

Add `config.active_record.warn_on_result_set_size` option

rafaelfranca merged commit 8b451e3 into rails:master Mar 26, 2015

kaspth changed the title ~~Add config.active_record.warn_on_result_set_size option~~ Add config.active_record.warn_on_records_fetched_greater_than option Mar 26, 2015

jonatack added a commit to jonatack/rails that referenced this pull request Mar 27, 2015

[skip ci] Update configuring.md with rails#18846

a1671af

Add `config.active_record.warn_on_records_fetched_greater_than` to the Configuring Rails Guide.

jonatack mentioned this pull request Mar 27, 2015

[skip ci] Update configuring.md with #18846 #19541

Merged

schneems added a commit that referenced this pull request Mar 27, 2015

Merge pull request #19541 from jonatack/warn-on-loading-large-query-sets

e1b967e

[skip ci] Update configuring.md with #18846

hundredwatt deleted the feat/warn-on-result-set-size branch March 27, 2015 12:19

aripollak reviewed Apr 6, 2015
View reviewed changes

rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

rafaelfranca mentioned this pull request Jul 8, 2021

ActiveRecord::Relation#destroy_all perform its work in batches #40445

Merged

		mattr_accessor :warn_on_result_set_size, instance_writer: false
		self.warn_on_result_set_size = nil

Add config.active_record.warn_on_records_fetched_greater_than option #18846

Add config.active_record.warn_on_records_fetched_greater_than option #18846

Uh oh!

Conversation

hundredwatt commented Feb 7, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hundredwatt commented Feb 7, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

kaspth commented Feb 8, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspth commented Feb 8, 2015

Uh oh!

kaspth commented Feb 8, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

kaspth commented Feb 8, 2015

Uh oh!

kaspth commented Feb 8, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

hundredwatt commented Feb 8, 2015

Uh oh!

kaspth commented Feb 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspth commented Feb 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hundredwatt commented Feb 11, 2015

Uh oh!

kaspth commented Feb 12, 2015

Uh oh!

hundredwatt commented Feb 13, 2015

Uh oh!

hundredwatt commented Feb 13, 2015

Uh oh!

kaspth commented Feb 14, 2015

Uh oh!

hundredwatt commented Feb 26, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspth commented Mar 25, 2015

Uh oh!

hundredwatt commented Mar 25, 2015

Uh oh!

hundredwatt commented Mar 25, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspth commented Mar 25, 2015

Uh oh!

hundredwatt commented Mar 25, 2015

Uh oh!

kaspth commented Mar 26, 2015

Uh oh!

hundredwatt commented Mar 27, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add `config.active_record.warn_on_records_fetched_greater_than` option #18846

Add `config.active_record.warn_on_records_fetched_greater_than` option #18846