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

Fix delete all to not produce sql in statement #14546

Merged

Conversation

@eileencodes
Copy link
Member

eileencodes commented Apr 1, 2014

When delete_all is run on a CollectionProxy and has a dependency of delete_all the SQL that is produced has an IN statement.

DELETE FROM associated_modelwhereassociated_model.parent_id= 1 ANDassociated_model.id IN (1, 2, 3...)

This is only happening if the association is not loaded — both loaded and non-loaded delete_all should behave the same. This is a problem when it comes to deleting many records because the query becomes very slow. Instead the SQL produced should be:

DELETE FROM associated_modelwhereassociated_model.parent_model_id=1

I fixed this by making sure the check for loaded and destroy also makes sure that the dependent is not delete_all, so the conditional goes to the else and deletes the records directly without the IN statement.

Thanks to @tenderlove for pairing with me to figure out exactly what was causing this issue.

eileencodes added 3 commits Mar 26, 2014
When delete_all is run on a CollectionProxy and has a
dependency of delete_all the SQL that is produced has an IN
statement. (DELETE FROM `associated_model` where `associated_model`
.`parent_id` = 1 AND `associated_model`.`id` IN (1, 2, 3...)).
This only happens if the association is not loaded (both loaded
and non-loaded delete_all should behave the same. This is a huge
problem when it comes to deleting many records because the query
becomes very slow. Instead the SQL produced should be (DELETE FROM
`assoicated_model` where `associated_model`.`parent_model_id`=1).

I fixed this by making sure the check for loaded and destroy also
makes sure that the dependent is not delete_all, so the conditional
goes to the else and deletes the records directly without the IN
statement.
Other methods compare specific patterns, this method outputs
the actual sql query that is generated.
delete_all sql if an association is not loaded should behave
the same as if the association is loaded. This test ensures
the SQL statements are exactly the same.
@eileencodes eileencodes changed the title Fix delete all to not use in statement Fix delete all to not produce sql in statement Apr 1, 2014
@@ -19,6 +19,12 @@ def assert_date_from_db(expected, actual, message = nil)
end
end

def capture_sql

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

Can we change assert_sql to use this new method?

This comment has been minimized.

Copy link
@tenderlove

tenderlove Apr 1, 2014

Member

We should, but this PR is just to fix a bug. We should have a different PR for refactoring. :)

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

If it is in a different commit I guess it is fine. Either way is fine to me

This comment has been minimized.

Copy link
@tenderlove

tenderlove Apr 1, 2014

Member

@eileencodes can you refactor assert_sql to use capture_sql and send another PR?

This comment has been minimized.

Copy link
@eileencodes

eileencodes Apr 1, 2014

Author Member

Sure thing! Actually threw it in there right before the merge request and a bunch of tests failed which I thought was odd, so I need to look at it some more.

I'll get everything working and send another PR.


author.thinking_posts.create!(:body => "test")
author.reload
author.thinking_posts.inspect

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

Do we need the inspect?

This comment has been minimized.

Copy link
@tenderlove

tenderlove Apr 1, 2014

Member

@rafaelfranca we need either the inspect or a to_a. The problem occurs when using irb, so I had @eileencodes use inspect.

This comment has been minimized.

Copy link
@rafaelfranca
@@ -248,7 +248,7 @@ def delete(*records)
dependent = _options[:dependent] || options[:dependent]

if records.first == :all
if loaded? || dependent == :destroy
if (loaded? || dependent == :destroy) && dependent != :delete_all

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

This conditional is weird. Should not it be:

if (loaded? && dependent != :delete_all) || dependent == :destroy

This comment has been minimized.

Copy link
@tenderlove

tenderlove Apr 1, 2014

Member

I don't know. @rafaelfranca the both read weird to me. 😅 If you have strong feelings about the conditional, we can change.

This comment has been minimized.

Copy link
@rafaelfranca

rafaelfranca Apr 1, 2014

Member

No strong feelings, it is just because if dependent is :destroy it is clearly != :delete_all. But we can merge as it is.

This comment has been minimized.

Copy link
@eileencodes

eileencodes Apr 1, 2014

Author Member

I can go either way, I don' have strong feelings towards either.

@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 1, 2014

:shipit:

tenderlove added a commit that referenced this pull request Apr 1, 2014
…IN_statement

Fix delete all to not produce sql in statement
@tenderlove tenderlove merged commit e9f53f7 into rails:master Apr 1, 2014
1 check was pending
1 check was pending
default The Travis CI build is in progress
Details
@rafaelfranca

This comment has been minimized.

Copy link
Member

rafaelfranca commented Apr 1, 2014

Thank you so much for working on this ❤️

@eileencodes

This comment has been minimized.

Copy link
Member Author

eileencodes commented Apr 1, 2014

No problem 😸

eileencodes added a commit to eileencodes/rails that referenced this pull request Apr 1, 2014
set assert_sql to reuse the capture_sql method from above
instead of repeating the code

in response to comments on issue rails#14546
robin850 added a commit to robin850/rails that referenced this pull request Apr 13, 2014
senny added a commit that referenced this pull request Apr 13, 2014
Add a changelog entry for #14546 [ci skip]
tenderlove added a commit that referenced this pull request Apr 14, 2014
* master: (70 commits)
  [ci skip] Added link to ruby-lang.org installation.
  Use the index on hidden field
  `collection_check_boxes` respects `:index` option for the hidden filed name.
  docs, double meaning of `serialize` argument. Closes #14284.
  Just call read_attribute, no need to use `send`.
  - Fix lingering reference to `:text` instead of the newer `:plain` - Section references `form_tag` instead of the `form_for` used in the example
  again, read_attribute is public, so just call it
  read_attribute is public, so we should just call it
  Disable assest cache store in docs [ci skip]
  Make counter cache decrementation on destroy idempotent
  Write the failing test case for concurrent counter cache
  [ci skip] Use plain underscore instead of "\_".
  Update documentation to use Rails.application instead
  Add a changelog entry for #14546 [ci skip]
  Move tests for deep_dup and duplicable to object directory
  Missing 'are' in note - [ci skip]
  CollectionHelpers now accepts a readonly option
  Fix a few typos [ci skip]
  Bundle tzinfo-data on :x64_mingw (64-bit Ruby on Windows).
  don't bother with an offset if the offset is zero
  ...
tenderlove added a commit that referenced this pull request Apr 16, 2014
…IN_statement

Fix delete all to not produce sql in statement
Conflicts:
	activerecord/test/cases/associations/has_many_associations_test.rb

Merge pull request #14727 from robin850/patch-17

Add a changelog entry for #14546 [ci skip]
Conflicts:
	activerecord/CHANGELOG.md
eileencodes added a commit to eileencodes/rails that referenced this pull request Apr 29, 2014
Nullify (or nil dependency) was doing the same thing delete_all
was doing in issue rails#14546, creating a large IN statement if
the association was loaded. Loaded and not loaded associations
should behave the same. The IN statement is also not great because
it's inefficient.
tenderlove added a commit that referenced this pull request May 14, 2014
…IN_statement

Fix delete all to not produce sql in statement
Conflicts:
	activerecord/test/cases/associations/has_many_associations_test.rb

Merge pull request #14727 from robin850/patch-17

Add a changelog entry for #14546 [ci skip]

Conflicts:
	activerecord/CHANGELOG.md

Conflicts:
	activerecord/CHANGELOG.md
	activerecord/lib/active_record/associations/collection_association.rb
	activerecord/test/cases/associations/has_many_associations_test.rb
	activerecord/test/cases/test_case.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.