From 8ad0ac2ae8d4e7e7fc75f23a946a2348e6f6d5b1 Mon Sep 17 00:00:00 2001 From: Greg Blomquist Date: Mon, 3 Apr 2017 10:12:55 -0400 Subject: [PATCH 1/5] Check for dirty records in save_inventory Allow save inventory to skip starting a transaction if the record being saved was not actually changed by the provider's inventory data. VMs and Storages still directly call `update_attributes!`. At the moment, it doesn't seem like those object counts are limiting factors. If that changes, we can look at applying the same change there. --- app/models/ems_refresh/save_inventory_helper.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/app/models/ems_refresh/save_inventory_helper.rb b/app/models/ems_refresh/save_inventory_helper.rb index d359a9581bd..f3b12918e8c 100644 --- a/app/models/ems_refresh/save_inventory_helper.rb +++ b/app/models/ems_refresh/save_inventory_helper.rb @@ -80,7 +80,7 @@ def save_inventory_single(type, parent, hash, child_keys = [], extra_keys = [], child_keys = Array.wrap(child_keys) remove_keys = Array.wrap(extra_keys) + child_keys + [:id] if child - child.update_attributes!(hash.except(:type, *remove_keys)) + update_attributes!(child, hash, [:type, *remove_keys]) else child = parent.send("create_#{type}!", hash.except(*remove_keys)) end @@ -94,12 +94,19 @@ def save_inventory_with_findkey(association, hash, deletes, new_records, record_ found = association.build(hash.except(:id)) new_records << found else - found.update_attributes!(hash.except(:id, :type)) + update_attributes!(found, hash, [:id, :type]) deletes.delete(found) unless deletes.blank? end found end + def update_attributes!(ar_model, attributes, remove_keys) + ar_model.assign_attributes(attributes.except(*remove_keys)) + ar_model.with_transaction_returning_status do + ar_model.save! if ar_model.changed? + end + end + def backup_keys(hash, keys) keys.each_with_object({}) { |k, backup| backup[k] = hash.delete(k) if hash.key?(k) } end From 4afb8a4e32d2c38563e52c6041c525d32433f602 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Thu, 6 Apr 2017 15:17:02 +0200 Subject: [PATCH 2/5] Do not do a blank transaction if the record hasn't changed Do not do a blank transaction if the record hasn't changed, becaiuse it causes extra 2 queries, transaction BEGIN and END --- app/models/ems_refresh/save_inventory_helper.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/ems_refresh/save_inventory_helper.rb b/app/models/ems_refresh/save_inventory_helper.rb index f3b12918e8c..deae019addd 100644 --- a/app/models/ems_refresh/save_inventory_helper.rb +++ b/app/models/ems_refresh/save_inventory_helper.rb @@ -102,9 +102,7 @@ def save_inventory_with_findkey(association, hash, deletes, new_records, record_ def update_attributes!(ar_model, attributes, remove_keys) ar_model.assign_attributes(attributes.except(*remove_keys)) - ar_model.with_transaction_returning_status do - ar_model.save! if ar_model.changed? - end + ar_model.save! if ar_model.changed? end def backup_keys(hash, keys) From 1ae950e8ca994b496f09a39aab783510307ad11b Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Thu, 6 Apr 2017 15:19:40 +0200 Subject: [PATCH 3/5] Wrap updating&deleting in 1 big transaction Wrap updating&deleting in 1 big transaction. Doing transaction per update is expensive, since we do BEGIN, update, END, so 3 queries per one updateand the same for delete. With remote DB, this will add a lot of processing time. --- .../ems_refresh/save_inventory_helper.rb | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/app/models/ems_refresh/save_inventory_helper.rb b/app/models/ems_refresh/save_inventory_helper.rb index deae019addd..bfca80cb0ad 100644 --- a/app/models/ems_refresh/save_inventory_helper.rb +++ b/app/models/ems_refresh/save_inventory_helper.rb @@ -53,17 +53,21 @@ def save_inventory_multi(association, hashes, deletes, find_key, child_keys = [] new_records = [] - hashes.each do |h| - found = save_inventory_with_findkey(association, h.except(*remove_keys), deletes_index, new_records, record_index) - save_child_inventory(found, h, child_keys) + ActiveRecord::Base.transaction do + hashes.each do |h| + found = save_inventory_with_findkey(association, h.except(*remove_keys), deletes_index, new_records, record_index) + save_child_inventory(found, h, child_keys) + end end # Delete the items no longer found deletes = deletes_index.values - unless deletes.blank? - type = association.proxy_association.reflection.name - _log.info("[#{type}] Deleting #{log_format_deletes(deletes)}") - disconnect ? deletes.each(&:disconnect_inv) : association.delete(deletes) + ActiveRecord::Base.transaction do + unless deletes.blank? + type = association.proxy_association.reflection.name + _log.info("[#{type}] Deleting #{log_format_deletes(deletes)}") + disconnect ? deletes.each(&:disconnect_inv) : association.delete(deletes) + end end # Add the new items From 77147ebff998d41a792b0494e5c51e68afef3592 Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 7 Apr 2017 09:47:15 +0200 Subject: [PATCH 4/5] Add a comment about rails issue causing nil transaction Add a comment about rails issue causing nil transaction --- app/models/ems_refresh/save_inventory_helper.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/ems_refresh/save_inventory_helper.rb b/app/models/ems_refresh/save_inventory_helper.rb index bfca80cb0ad..2139a782e7d 100644 --- a/app/models/ems_refresh/save_inventory_helper.rb +++ b/app/models/ems_refresh/save_inventory_helper.rb @@ -106,6 +106,7 @@ def save_inventory_with_findkey(association, hash, deletes, new_records, record_ def update_attributes!(ar_model, attributes, remove_keys) ar_model.assign_attributes(attributes.except(*remove_keys)) + # HACK: Avoid empty BEGIN/COMMIT pair until fix is made for https://github.com/rails/rails/issues/17937 ar_model.save! if ar_model.changed? end From 2b2862eb1847ce05009b335163e5aa54bdd3993d Mon Sep 17 00:00:00 2001 From: Ladislav Smola Date: Fri, 7 Apr 2017 09:55:13 +0200 Subject: [PATCH 5/5] Do a transaction only if there are items to be deleted Do a transaction only if there are items to be deleted, to avoind empty transactions. --- app/models/ems_refresh/save_inventory_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ems_refresh/save_inventory_helper.rb b/app/models/ems_refresh/save_inventory_helper.rb index 2139a782e7d..9d6e32e9a9f 100644 --- a/app/models/ems_refresh/save_inventory_helper.rb +++ b/app/models/ems_refresh/save_inventory_helper.rb @@ -62,8 +62,8 @@ def save_inventory_multi(association, hashes, deletes, find_key, child_keys = [] # Delete the items no longer found deletes = deletes_index.values - ActiveRecord::Base.transaction do - unless deletes.blank? + unless deletes.blank? + ActiveRecord::Base.transaction do type = association.proxy_association.reflection.name _log.info("[#{type}] Deleting #{log_format_deletes(deletes)}") disconnect ? deletes.each(&:disconnect_inv) : association.delete(deletes)