From c24642d793d680c96e6acc1e6468bba9b770d706 Mon Sep 17 00:00:00 2001 From: Arthur Neves Date: Wed, 12 Jun 2019 16:03:58 -0400 Subject: [PATCH 1/4] Inverse the order of writes on statesman transitions StatesMan will update the old transitions to `most_recent: nil` and then insert the new transition as `most_recent: true`. However that can cause deadlocks on MySQL when running concurrent state transitions. On the update, MySQL will hold a gap lock to the unique indexes there are, so other transactions cannot insert. After two updates on the most_recent and two inserts, the second insert will fail with a deadlock. For more explanation see the PR description that includes this commit. --- lib/statesman/adapters/active_record.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/statesman/adapters/active_record.rb b/lib/statesman/adapters/active_record.rb index 654f7ce7..72ecbeff 100644 --- a/lib/statesman/adapters/active_record.rb +++ b/lib/statesman/adapters/active_record.rb @@ -68,14 +68,16 @@ def create_transition(from, to, metadata) sort_key: next_sort_key, metadata: metadata } - transition_attributes[:most_recent] = true - transition = transitions_for_parent.build(transition_attributes) ::ActiveRecord::Base.transaction(requires_new: true) do @observer.execute(:before, from, to, transition) - unset_old_most_recent + # We save the transition first, and mark it as + # most_recent after to avoid letting MySQL put a + # next-key lock which could cause deadlocks. transition.save! + unset_old_most_recent + transition.update!(most_recent: true) @last_transition = transition @observer.execute(:after, from, to, transition) add_after_commit_callback(from, to, transition) From 753649d244f436ddddf8d9bf0fb69b637bd95997 Mon Sep 17 00:00:00 2001 From: Arthur Neves Date: Wed, 12 Jun 2019 16:54:26 -0400 Subject: [PATCH 2/4] Make sure the initial most_recent state is false/nil --- lib/statesman/adapters/active_record.rb | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/lib/statesman/adapters/active_record.rb b/lib/statesman/adapters/active_record.rb index 72ecbeff..0b973346 100644 --- a/lib/statesman/adapters/active_record.rb +++ b/lib/statesman/adapters/active_record.rb @@ -64,11 +64,9 @@ def last(force_reload: false) private def create_transition(from, to, metadata) - transition_attributes = { to_state: to, - sort_key: next_sort_key, - metadata: metadata } - - transition = transitions_for_parent.build(transition_attributes) + transition = transitions_for_parent.build( + default_transition_attributes(to, metadata), + ) ::ActiveRecord::Base.transaction(requires_new: true) do @observer.execute(:before, from, to, transition) @@ -86,6 +84,20 @@ def create_transition(from, to, metadata) transition end + def default_transition_attributes(to, metadata) + transition_attributes = { to_state: to, + sort_key: next_sort_key, + metadata: metadata } + + # see comment on `unset_old_most_recent` method + if transition_class.columns_hash["most_recent"].null == false + transition_attributes[:most_recent] = false + else + transition_attributes[:most_recent] = nil + end + transition_attributes + end + def add_after_commit_callback(from, to, transition) ::ActiveRecord::Base.connection.add_transaction_record( ActiveRecordAfterCommitWrap.new do From fa39d5dc7524d68b2650ff9c08800cf6ffe93e33 Mon Sep 17 00:00:00 2001 From: Lawrence Jones Date: Thu, 13 Jun 2019 12:25:15 +0100 Subject: [PATCH 3/4] Lock-lesser transitions without additional queries https://github.com/gocardless/statesman/pull/350 @arthurnn opened #350 to reduce the impact of Statesman taking gap locks when using MySQL. The crux of the issue is that MySQL's implementation of REPEATABLE READ can take wide locks when an update touches no rows, which happens frequently on the first transition of Statesman. By first creating the new transition, we can avoid issuing an update that will take the large gap lock. This order of queries meant we added an additional query to the transition step which could impact people who rely on low-latency Statesman transitions. This commit is another take on the same approach that collapses two queries into one, by taking the update of the old and new transition's most_recent column and updating them together. It's slightly janky but if robust, would be a good alternative to avoid additional latency. --- lib/statesman/adapters/active_record.rb | 99 ++++++++++++++++++------- 1 file changed, 72 insertions(+), 27 deletions(-) diff --git a/lib/statesman/adapters/active_record.rb b/lib/statesman/adapters/active_record.rb index 0b973346..dabd26b0 100644 --- a/lib/statesman/adapters/active_record.rb +++ b/lib/statesman/adapters/active_record.rb @@ -63,6 +63,7 @@ def last(force_reload: false) private + # rubocop:disable Metrics/MethodLength def create_transition(from, to, metadata) transition = transitions_for_parent.build( default_transition_attributes(to, metadata), @@ -70,12 +71,20 @@ def create_transition(from, to, metadata) ::ActiveRecord::Base.transaction(requires_new: true) do @observer.execute(:before, from, to, transition) - # We save the transition first, and mark it as - # most_recent after to avoid letting MySQL put a - # next-key lock which could cause deadlocks. + + # We save the transition first with most_recent falsy, then mark most_recent + # true after to avoid letting MySQL acquire a next-key lock which can cause + # deadlocks. + # + # To avoid an additional query, we manually adjust the most_recent attribute on + # our transition assuming that update_most_recents will have set it to true. transition.save! - unset_old_most_recent - transition.update!(most_recent: true) + unless update_most_recents(transition.id) > 0 + raise ActiveRecord::Rollback, "failed to update most_recent" + end + + transition.assign_attributes(most_recent: true) + @last_transition = transition @observer.execute(:after, from, to, transition) add_after_commit_callback(from, to, transition) @@ -83,6 +92,7 @@ def create_transition(from, to, metadata) transition end + # rubocop:enable Metrics/MethodLength def default_transition_attributes(to, metadata) transition_attributes = { to_state: to, @@ -110,21 +120,58 @@ def transitions_for_parent parent_model.send(@association_name) end - def unset_old_most_recent - most_recent = transitions_for_parent.where(most_recent: true) + # Sets the given transition most_recent = t while unsetting the most_recent of any + # previous transitions. + def update_most_recents(most_recent_id) + transitions = transitions_for_parent + last_or_current = transitions.where(id: most_recent_id).or( + transitions.where(most_recent: true) + ) - # Check whether the `most_recent` column allows null values. If it - # doesn't, set old records to `false`, otherwise, set them to `NULL`. - # - # Some conditioning here is required to support databases that don't - # support partial indexes. By doing the conditioning on the column, - # rather than Rails' opinion of whether the database supports partial - # indexes, we're robust to DBs later adding support for partial indexes. - if transition_class.columns_hash["most_recent"].null == false - most_recent.update_all(with_updated_timestamp(most_recent: false)) - else - most_recent.update_all(with_updated_timestamp(most_recent: nil)) + last_or_current.update_all( + build_most_recents_update_all(most_recent_id), + ) + end + + # Generates update_all parameters that will touch the updated timestamp (if valid + # for this model) and ensure only the transition with the most_recent_id has + # most_recent set to true. + # + # This is quite nasty, but combines two updates (set all most_recent = f, set + # current most_recent = t) into one, which helps improve transition performance + # especially when database latency is significant. + # + # The SQL this can help produce looks like: + # + # update transitions + # set most_recent = (case when id = 'PA123' then TRUE else FALSE end) + # , updated_at = '...' + # ... + # + def build_most_recents_update_all(most_recent_id) + clause = "most_recent = (case when id = ? then ? else ? end)" + parameters = [most_recent_id, true, not_most_recent_value] + + updated_column, updated_at = updated_timestamp + if updated_column + clause += ", #{updated_column} = ?" + parameters.push(updated_at) end + + [clause, *parameters] + end + + # Check whether the `most_recent` column allows null values. If it doesn't, set old + # records to `false`, otherwise, set them to `NULL`. + # + # Some conditioning here is required to support databases that don't support partial + # indexes. By doing the conditioning on the column, rather than Rails' opinion of + # whether the database supports partial indexes, we're robust to DBs later adding + # support for partial indexes. + def not_most_recent_value + return false if transition_class.columns_hash["most_recent"].null == false + + nil end def next_sort_key @@ -180,7 +227,8 @@ def association_join_primary_key(association) end end - def with_updated_timestamp(params) + # updated_timestamp should return [column_name, value] + def updated_timestamp # TODO: Once we've set expectations that transition classes should conform to # the interface of Adapters::ActiveRecordTransition as a breaking change in the # next major version, we can stop calling `#respond_to?` first and instead @@ -194,15 +242,12 @@ def with_updated_timestamp(params) ActiveRecordTransition::DEFAULT_UPDATED_TIMESTAMP_COLUMN end - return params if column.nil? - - timestamp = if ::ActiveRecord::Base.default_timezone == :utc - Time.now.utc - else - Time.now - end + # No updated timestamp column, don't return anything + return nil if column.nil? - params.merge(column => timestamp) + [ + column, ::ActiveRecord::Base.default_timezone == :utc ? Time.now.utc : Time.now, + ] end end From 353f1b1fcc0c24b5c3638c8506f7a26626e5e397 Mon Sep 17 00:00:00 2001 From: Arthur Neves Date: Thu, 13 Jun 2019 12:40:09 -0400 Subject: [PATCH 4/4] fmt --- lib/statesman/adapters/active_record.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/statesman/adapters/active_record.rb b/lib/statesman/adapters/active_record.rb index dabd26b0..a2713cc6 100644 --- a/lib/statesman/adapters/active_record.rb +++ b/lib/statesman/adapters/active_record.rb @@ -125,7 +125,7 @@ def transitions_for_parent def update_most_recents(most_recent_id) transitions = transitions_for_parent last_or_current = transitions.where(id: most_recent_id).or( - transitions.where(most_recent: true) + transitions.where(most_recent: true), ) last_or_current.update_all( @@ -246,7 +246,7 @@ def updated_timestamp return nil if column.nil? [ - column, ::ActiveRecord::Base.default_timezone == :utc ? Time.now.utc : Time.now, + column, ::ActiveRecord::Base.default_timezone == :utc ? Time.now.utc : Time.now ] end end