Lock the parent object to prevent deadlocks updating most_recent

[pjungwir]

In unset_old_most_recent it is easy to get deadlocks from this code:

transitions_for_parent.update_all(most_recent: false)

The problem is that it updates several rows, and concurrent updates may not update the rows in the same order. This causes deadlocks in Postgres.

This commit fixes the problem by locking the parent object before altering its status transitions.

I wasn't really sure how to write a test for this, although in my own project I could reproduce the problem right away by running this in two shells at once:

# script/c.rb
# run with `rails runner script/c.rb`
c = Contact.find 20159
while true do
  10.times do
    c.state_machine.trigger(:archive)
    c.state_machine.trigger(:activate)
  end
  print "."
  STDOUT.flush
end

[pjungwir]

Oh, note that even with this commit, running that script twice simultaneously will still give you a Statesman::TransitionConflictError. But I believe that is correct and desirable, to avoid invalid histories like this:

2016-01-03 14:03:00 transition from active to archived
2016-01-03 14:03:01 transition from active to archived

And that exception is a lot better than a deadlock!

[hmac]

@pjungwir sorry for the late response! This looks pretty legit to me - I can reproduce the deadlock and your fix seems to do the trick. Will do a bit more testing on this and hopefully merge it soon.

[hmac]

Is there a reason to lock here (around the whole transaction) rather than just around unset_most_recent?

[hmac]

Actually this is probably the best option. Any locks acquired will be held until the end of the transaction anyway (I think).

[hmac]

Note that this is a breaking change due to the fact that with_lock implicitly reloads the model.

This will no longer do what you’d expect:

    model.foo = "bar"
    model.transition_to!(:some_state)
    model.save!

model.transition_to! will reload the model, and you’ll lose the mutation from the first line.

[santi-h]

@pjungwir I haven't tested this yet but you could try acquiring the locks in the same order in L98 and L100 and that way this wouldn't be a breaking change:

if transition_class.columns_hash['most_recent'].null == false
  update_fields = { most_recent: false }
else
  update_fields = { most_recent: nil }
end
transitions_for_parent.order("`#{@association_name}`.`id` ASC").update_all(update_fields)

In my case, this is changing the query from

UPDATE `rental_photo_set_transitions` SET `rental_photo_set_transitions`.`most_recent` = 0 WHERE `rental_photo_set_transitions`.`rental_photo_set_id` = 6

to

UPDATE `rental_photo_set_transitions` SET `rental_photo_set_transitions`.`most_recent` = 0 WHERE `rental_photo_set_transitions`.`rental_photo_set_id` = 6 ORDER BY id ASC

If the ORDER BY clause is specified, the rows are updated in the order that is specified

• https://dev.mysql.com/doc/refman/5.7/en/update.html

So I'm assuming it also tries to obtain the locks in the order that is specified.

[pjungwir]

Postgres doesn't support ORDER BY for UPDATE. For example:

=> update users set email = email order by id;
ERROR:  syntax error at or near "order"

It has come up on the mailing list before though:

https://www.postgresql.org/message-id/426DB1A0.2000604%40carvalhaes.net

https://www.postgresql.org/message-id/freemail.20070030161126.43285%40fm10.freemail.hu

[pjungwir]

Here is a thread about it I started myself a few years ago: :-)

https://www.postgresql.org/message-id/CA%2B6hpa%3D9cV9L%2BspfK_yn%2BrTeaQtHWyLLXWtLgpKJi%2BUJYUc2Qw%40mail.gmail.com

[santi-h]

Postgres doesn't support ORDER BY for UPDATE

:(