Rails Ruby on Rails → 2.x

Comments and changes to this ticket

• 

  Pratik March 10th, 2009 @ 12:40 PM

  • Assigned user set to “Pratik”
  • State changed from “new” to “wontfix”

  You shouldnt really be calling .destroy on a new record. But in any case, a patch would be great and we can reevaluate this :) - http://guides.rails.info/contrib...

  Thanks !

• You flagged this item as spam.
  

  Richie Vos March 11th, 2009 @ 02:32 AM

  • Tag set to “active_record, associations, destroy, new_record, patch”

  Attaching a patch that puts unless new_record? checks in the before_destroy hooks for :nullify and :delete_all.

  I do not believe these are necessary for :destroy since in that case its either not going to find any associated records, or if it does it probably should be calling .destroy on them.

  As I noted in the commit message, the behavior of performing these writes can cause db deadlocks (at least it was the cause of some in our app).

  ---

  When running a dependent destroy, first check if we're a new record before firing off db commands to prevent WHERE foreign_key = NULL statements from running.

  Not only does this prevent an extra db connection, it also can prevent some bad behavior from occurring when you have rows where the foreign_key actually is set to null. In a nullify case, if you have 2 threads both trying to do something like UPDATE cars SET manu facturer_id = NULL WHERE manufacturer_id = NULL, you can run into deadlocks since both may require a large db read and write.

  • 0001-When-running-a-dependent-destroy-first-check-if-we.patch 5.4 KB

• You flagged this item as spam.
  

  Richie Vos March 11th, 2009 @ 02:34 AM

  Pratik, due to the patch could you re-open this?

• You flagged this item as spam.
  

  Richie Vos March 11th, 2009 @ 02:44 AM

  Also wanted to mention, while I understand some gain comes from adding comments around string interpolations, things like this seem like they're pushing it:

  
  before_destroy do |record|                    # before_destroy do |record|
    unless record.new_record?                   #   unless record.new_record?
      nullify_has_many_dependencies(record,     #     nullify_has_many_dependencies(record,
        "#{reflection.name}",                   #       "posts",
        #{reflection.class_name},               #       Post,
        "#{reflection.primary_key_name}",       #       "user_id",
        %@#{dependent_conditions}@)             #       %@...@) # this is a string literal like %(...)
    end                                         #   end
  end                                           # end
  
  

  Because of those comments, to make my change I had to update 6 lines of code instead of just putting a 1-line "unless record.new_record?" in, which seems non-optimal.