Skip to content

:dependent => :delete_all malfunctioning in 3.1 #3672

Closed
njakobsen opened this Issue Nov 17, 2011 · 6 comments

3 participants

@njakobsen

Here's the set up

class Discussion < ActiveRecord::Base
    has_many :posts, :dependent => :delete_all
end

class Post < ActiveRecord::Base
    belongs_to :discussion
end

In Rails 3.0, calling Discussion.find(1).destroy will perform an sql delete as follows

DELETE FROM "posts" WHERE ((discussion_id = 1))
DELETE FROM "discussions" WHERE ("discussion"."id" = 1)

In Rails 3.1, calling the same thing will perform the following

Post Load (247.1ms)  SELECT "posts".* FROM "posts" WHERE "posts"."discussion_id" = 1
DELETE FROM "posts" WHERE "posts"."discussion_id" = 1 AND "posts"."id" IN (778876, 778928, 779125, 779175, 779223, 788836, etc...)
DELETE FROM "discussions" WHERE "discussions"."id" = $1  [["id", 1]]

You can create a new Rails 3.1 app and this strange behaviour will happen. This is broken because if you have many posts, ruby eats your CPU alive.

@jonleighton
Ruby on Rails member
@jonleighton jonleighton was assigned Nov 18, 2011
@njakobsen

Here's a gist to test the behaviour in your ActiveRecord of choice.

https://gist.github.com/1375294

@jonleighton jonleighton added a commit that closed this issue Nov 19, 2011
@jonleighton jonleighton Perf fix
If we're deleting all records in an association, don't add a IN(..)
clause to the query.

Fixes #3672.
cb0c3e4
@andhapp andhapp pushed a commit to andhapp/rails that referenced this issue Nov 19, 2011
@jonleighton jonleighton Perf fix
If we're deleting all records in an association, don't add a IN(..)
clause to the query.

Fixes #3672.
fec85cf
@njakobsen

I'm still not sure it's fixed. It no longer puts the Post ids in the query but it still does an sql query and I'm pretty sure it tries to instantiate them as part of the delete. See the log output now.

Discussion Load (0.1ms)  SELECT "discussions".* FROM "discussions" LIMIT 1
Post Load (20.4ms)  SELECT "posts".* FROM "posts" WHERE "posts"."discussion_id" = 1
SQL (12.4ms)  DELETE FROM "posts" WHERE "posts"."discussion_id" = 1
****HANGS for a long time****
SQL (0.1ms)  DELETE FROM "discussions" WHERE "discussions"."id" = ?  [["id", 1]]

During the delete with 10000 posts, irb hangs for many seconds after the DELETE FROM "posts" and then outputs the last log line and returns control back to the user. Under rails 3.0 the deletion is near instantaneous. There should be no reason to Post Load which is the whole idea behind the :delete_all option.

@jonleighton jonleighton reopened this Nov 23, 2011
@jonleighton
Ruby on Rails member

You're right. I will take another look at this. (Or feel free to send a patch.)

@njakobsen

I would love to be able to do more than just poke you when it's broken. :) Unfortunately, right my schedule doesn't leave me with much time to get up to speed on activerecord. Thanks for looking into it for me. In the mean time I've worked around it using an after_destroy that does a manual delete_all.

@jonleighton jonleighton added a commit that closed this issue Nov 23, 2011
@jonleighton jonleighton Perf fix
If we're deleting all records in an association, don't add a IN(..)
clause to the query.

Fixes #3672.
fec85cf
@rywall
rywall commented Nov 24, 2011

I don't think this should have been closed again. It looks like we might have found an issue in github issues along the way?

@jonleighton jonleighton reopened this Nov 24, 2011
@jonleighton jonleighton added a commit that closed this issue Dec 14, 2011
@jonleighton jonleighton Fix #3672 again (dependent: delete_all perf)
Conflicts:

	activerecord/lib/active_record/associations/builder/has_many.rb
	activerecord/lib/active_record/associations/has_many_association.rb
b6ae05e
@lest lest pushed a commit to lest/rails that referenced this issue Dec 14, 2011
@jonleighton jonleighton Fix #3672 again (dependent: delete_all perf) 889e8be
@ttosch ttosch pushed a commit that referenced this issue Jan 19, 2015
@jonleighton jonleighton Perf fix
If we're deleting all records in an association, don't add a IN(..)
clause to the query.

Fixes #3672.
bb22bfa
@ttosch ttosch pushed a commit that referenced this issue Jan 19, 2015
@jonleighton jonleighton Fix #3672 again (dependent: delete_all perf)
Conflicts:

	activerecord/lib/active_record/associations/builder/has_many.rb
	activerecord/lib/active_record/associations/has_many_association.rb
9b6e3df
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.