From 7327f20d5ef05816fce5462c84a2ca093e8e1f3d Mon Sep 17 00:00:00 2001 From: Adrianna Chang Date: Fri, 15 Mar 2024 16:39:12 -0400 Subject: [PATCH] Retry known idempotent SELECT queries on connection-related exceptions This commit makes two types of queries retry-able by opting into our `allow_retry` flag: 1) SELECT queries we construct by walking the Arel tree via `#to_sql_and_binds`. We use a new `retryable` attribute on collector classes, which defaults to true for most node types, but will be set to false for non-idempotent node types (functions, SQL literals, etc). The `retryable` value is returned from `#to_sql_and_binds` and used by `#select_all` and passed down the call stack, eventually reaching the adapter's `#internal_exec_query` method. Internally-generated SQL literals are marked as retryable via a new `retryable` attribute on `Arel::Nodes::SqlLiteral`. 2) `#find` and `#find_by` queries with known attributes. We set `allow_retry: true` in `#cached_find_by`, and pass this down to `#find_by_sql` and `#_query_by_sql`. These changes ensure that queries we know are safe to retry can be retried automatically. --- activerecord/CHANGELOG.md | 9 +++ .../join_dependency/join_association.rb | 2 +- .../abstract/database_statements.rb | 26 +++++--- .../abstract/query_cache.rb | 8 +-- .../abstract_mysql_adapter.rb | 4 +- .../mysql/database_statements.rb | 2 +- .../mysql2/database_statements.rb | 4 +- .../postgresql/database_statements.rb | 2 +- .../connection_adapters/postgresql_adapter.rb | 4 +- .../sqlite3/database_statements.rb | 4 +- .../trilogy/database_statements.rb | 4 +- activerecord/lib/active_record/core.rb | 2 +- .../lib/active_record/internal_metadata.rb | 2 +- activerecord/lib/active_record/querying.rb | 8 +-- .../active_record/relation/calculations.rb | 4 +- .../relation/predicate_builder.rb | 4 +- .../active_record/relation/query_methods.rb | 2 +- .../lib/active_record/statement_cache.rb | 6 +- activerecord/lib/arel.rb | 10 ++- activerecord/lib/arel/alias_predication.rb | 2 +- activerecord/lib/arel/collectors/bind.rb | 2 + activerecord/lib/arel/collectors/composite.rb | 7 ++ .../lib/arel/collectors/sql_string.rb | 2 +- .../lib/arel/collectors/substitute_binds.rb | 2 +- activerecord/lib/arel/nodes/sql_literal.rb | 7 ++ activerecord/lib/arel/select_manager.rb | 2 +- activerecord/lib/arel/visitors/mysql.rb | 4 +- activerecord/lib/arel/visitors/to_sql.rb | 6 ++ activerecord/test/cases/adapter_test.rb | 66 ++++++++++++++++++- .../cases/arel/collectors/composite_test.rb | 10 +++ .../test/cases/arel/visitors/to_sql_test.rb | 56 ++++++++++++++++ 31 files changed, 221 insertions(+), 52 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index b7f1dc9ef104..b087a7f708d5 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,12 @@ +* Retry known idempotent SELECT queries on connection-related exceptions + + SELECT queries we construct by walking the Arel tree and / or with known model attributes + are idempotent and can safely be retried in the case of a connection error. Previously, + adapters such as `TrilogyAdapter` would raise `ActiveRecord::ConnectionFailed: Trilogy::EOFError` + when encountering a connection error mid-request. + + *Adrianna Chang* + * Add dirties option to uncached This adds a `dirties` option to `ActiveRecord::Base.uncached` and diff --git a/activerecord/lib/active_record/associations/join_dependency/join_association.rb b/activerecord/lib/active_record/associations/join_dependency/join_association.rb index bd87870a3eb4..809d8e0455a9 100644 --- a/activerecord/lib/active_record/associations/join_dependency/join_association.rb +++ b/activerecord/lib/active_record/associations/join_dependency/join_association.rb @@ -93,7 +93,7 @@ def strict_loading? def append_constraints(connection, join, constraints) if join.is_a?(Arel::Nodes::StringJoin) join_string = Arel::Nodes::And.new(constraints.unshift join.left) - join.left = Arel.sql(connection.visitor.compile(join_string)) + join.left = Arel.sql(connection.visitor.compile(join_string), retryable: true) else right = join.right right.expr = Arel::Nodes::And.new(constraints.unshift right.expr) diff --git a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb index 62e925a31bbd..3e1ea4cf4de8 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb @@ -14,7 +14,7 @@ def to_sql(arel_or_sql_string, binds = []) sql end - def to_sql_and_binds(arel_or_sql_string, binds = [], preparable = nil) # :nodoc: + def to_sql_and_binds(arel_or_sql_string, binds = [], preparable = nil, allow_retry = false) # :nodoc: # Arel::TreeManager -> Arel::Node if arel_or_sql_string.respond_to?(:ast) arel_or_sql_string = arel_or_sql_string.ast @@ -27,6 +27,7 @@ def to_sql_and_binds(arel_or_sql_string, binds = [], preparable = nil) # :nodoc: end collector = collector() + collector.retryable = true if prepared_statements collector.preparable = true @@ -41,10 +42,11 @@ def to_sql_and_binds(arel_or_sql_string, binds = [], preparable = nil) # :nodoc: else sql = visitor.compile(arel_or_sql_string, collector) end - [sql.freeze, binds, preparable] + allow_retry = collector.retryable + [sql.freeze, binds, preparable, allow_retry] else arel_or_sql_string = arel_or_sql_string.dup.freeze unless arel_or_sql_string.frozen? - [arel_or_sql_string, binds, preparable] + [arel_or_sql_string, binds, preparable, allow_retry] end end private :to_sql_and_binds @@ -64,11 +66,15 @@ def cacheable_query(klass, arel) # :nodoc: end # Returns an ActiveRecord::Result instance. - def select_all(arel, name = nil, binds = [], preparable: nil, async: false) + def select_all(arel, name = nil, binds = [], preparable: nil, async: false, allow_retry: false) arel = arel_from_relation(arel) - sql, binds, preparable = to_sql_and_binds(arel, binds, preparable) + sql, binds, preparable, allow_retry = to_sql_and_binds(arel, binds, preparable, allow_retry) - select(sql, name, binds, prepare: prepared_statements && preparable, async: async && FutureResult::SelectAll) + select(sql, name, binds, + prepare: prepared_statements && preparable, + async: async && FutureResult::SelectAll, + allow_retry: allow_retry + ) rescue ::RangeError ActiveRecord::Result.empty(async: async) end @@ -495,7 +501,7 @@ def with_yaml_fallback(value) # :nodoc: end # This is a safe default, even if not high precision on all databases - HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP").freeze # :nodoc: + HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP", retryable: true).freeze # :nodoc: private_constant :HIGH_PRECISION_CURRENT_TIMESTAMP # Returns an Arel SQL literal for the CURRENT_TIMESTAMP for usage with @@ -507,7 +513,7 @@ def high_precision_current_timestamp HIGH_PRECISION_CURRENT_TIMESTAMP end - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: raise NotImplementedError end @@ -606,7 +612,7 @@ def combine_multi_statements(total_sql) end # Returns an ActiveRecord::Result instance. - def select(sql, name = nil, binds = [], prepare: false, async: false) + def select(sql, name = nil, binds = [], prepare: false, async: false, allow_retry: false) if async && async_enabled? if current_transaction.joinable? raise AsynchronousQueryInsideTransactionError, "Asynchronous queries are not allowed inside transactions" @@ -627,7 +633,7 @@ def select(sql, name = nil, binds = [], prepare: false, async: false) return future_result end - result = internal_exec_query(sql, name, binds, prepare: prepare) + result = internal_exec_query(sql, name, binds, prepare: prepare, allow_retry: allow_retry) if async FutureResult.wrap(result) else diff --git a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb index c52fdf32a74c..9edae461af04 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb @@ -204,19 +204,19 @@ def clear_query_cache pool.clear_query_cache end - def select_all(arel, name = nil, binds = [], preparable: nil, async: false) # :nodoc: + def select_all(arel, name = nil, binds = [], preparable: nil, async: false, allow_retry: false) # :nodoc: arel = arel_from_relation(arel) # If arel is locked this is a SELECT ... FOR UPDATE or somesuch. # Such queries should not be cached. if @query_cache&.enabled? && !(arel.respond_to?(:locked) && arel.locked) - sql, binds, preparable = to_sql_and_binds(arel, binds, preparable) + sql, binds, preparable, allow_retry = to_sql_and_binds(arel, binds, preparable) if async - result = lookup_sql_cache(sql, name, binds) || super(sql, name, binds, preparable: preparable, async: async) + result = lookup_sql_cache(sql, name, binds) || super(sql, name, binds, preparable: preparable, async: async, allow_retry: allow_retry) FutureResult.wrap(result) else - cache_sql(sql, name, binds) { super(sql, name, binds, preparable: preparable, async: async) } + cache_sql(sql, name, binds) { super(sql, name, binds, preparable: preparable, async: async, allow_retry: allow_retry) } end else super diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index d875e1b482b1..63ce81d16944 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -229,12 +229,12 @@ def disable_referential_integrity # :nodoc: # Mysql2Adapter doesn't have to free a result after using it, but we use this method # to write stuff in an abstract way without concerning ourselves about whether it # needs to be explicitly freed or not. - def execute_and_free(sql, name = nil, async: false) # :nodoc: + def execute_and_free(sql, name = nil, async: false, allow_retry: false) # :nodoc: sql = transform_query(sql) check_if_write_query(sql) mark_transaction_written_if_write(sql) - yield raw_execute(sql, name, async: async) + yield raw_execute(sql, name, async: async, allow_retry: allow_retry) end def begin_db_transaction # :nodoc: diff --git a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb index a5abb36c9ee4..a358cd2b2b6a 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb @@ -11,7 +11,7 @@ module DatabaseStatements # https://dev.mysql.com/doc/refman/5.7/en/date-and-time-functions.html#function_current-timestamp # https://dev.mysql.com/doc/refman/5.7/en/date-and-time-type-syntax.html - HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP(6)").freeze # :nodoc: + HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP(6)", retryable: true).freeze # :nodoc: private_constant :HIGH_PRECISION_CURRENT_TIMESTAMP def write_query?(sql) # :nodoc: diff --git a/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb b/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb index ce79cd5a8853..7d4e4105da2e 100644 --- a/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/mysql2/database_statements.rb @@ -18,9 +18,9 @@ def select_all(*, **) # :nodoc: result end - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: if without_prepared_statement?(binds) - execute_and_free(sql, name, async: async) do |result| + execute_and_free(sql, name, async: async, allow_retry: allow_retry) do |result| if result build_result(columns: result.fields, rows: result.to_a) else diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb index 6fb13f0c54f1..7ece6a11bca6 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb @@ -124,7 +124,7 @@ def exec_restart_db_transaction # :nodoc: end # From https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-CURRENT - HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP").freeze # :nodoc: + HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("CURRENT_TIMESTAMP", retryable: true).freeze # :nodoc: private_constant :HIGH_PRECISION_CURRENT_TIMESTAMP def high_precision_current_timestamp diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 8dcfc85767f9..f1962ff49409 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -881,7 +881,7 @@ def exec_no_cache(sql, name, binds, async:, allow_retry:, materialize_transactio type_casted_binds = type_casted_binds(binds) log(sql, name, binds, type_casted_binds, async: async) do |notification_payload| - with_raw_connection(allow_retry: false, materialize_transactions: materialize_transactions) do |conn| + with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| result = conn.exec_params(sql, type_casted_binds) verified! notification_payload[:row_count] = result.count @@ -895,7 +895,7 @@ def exec_cache(sql, name, binds, async:, allow_retry:, materialize_transactions: update_typemap_for_default_timezone - with_raw_connection(allow_retry: false, materialize_transactions: materialize_transactions) do |conn| + with_raw_connection(allow_retry: allow_retry, materialize_transactions: materialize_transactions) do |conn| stmt_key = prepare_statement(sql, binds, conn) type_casted_binds = type_casted_binds(binds) diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb index a91f08b6f15f..03453b48f4b1 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3/database_statements.rb @@ -21,7 +21,7 @@ def explain(arel, binds = [], _options = []) SQLite3::ExplainPrettyPrinter.new.pp(result) end - def internal_exec_query(sql, name = nil, binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = nil, binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: sql = transform_query(sql) check_if_write_query(sql) @@ -106,7 +106,7 @@ def exec_rollback_db_transaction # :nodoc: # https://stackoverflow.com/questions/17574784 # https://www.sqlite.org/lang_datefunc.html - HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')").freeze # :nodoc: + HIGH_PRECISION_CURRENT_TIMESTAMP = Arel.sql("STRFTIME('%Y-%m-%d %H:%M:%f', 'NOW')", retryable: true).freeze # :nodoc: private_constant :HIGH_PRECISION_CURRENT_TIMESTAMP def high_precision_current_timestamp diff --git a/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb b/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb index 334109dd0d0d..3e583f767ff7 100644 --- a/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb +++ b/activerecord/lib/active_record/connection_adapters/trilogy/database_statements.rb @@ -12,12 +12,12 @@ def select_all(*, **) # :nodoc: result end - def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false) # :nodoc: + def internal_exec_query(sql, name = "SQL", binds = [], prepare: false, async: false, allow_retry: false) # :nodoc: sql = transform_query(sql) check_if_write_query(sql) mark_transaction_written_if_write(sql) - result = raw_execute(sql, name, async: async) + result = raw_execute(sql, name, async: async, allow_retry: allow_retry) ActiveRecord::Result.new(result.fields, result.to_a) end diff --git a/activerecord/lib/active_record/core.rb b/activerecord/lib/active_record/core.rb index dad3354ef2bb..7510cea6aa9c 100644 --- a/activerecord/lib/active_record/core.rb +++ b/activerecord/lib/active_record/core.rb @@ -431,7 +431,7 @@ def cached_find_by(keys, values) } begin - statement.execute(values.flatten, lease_connection).first + statement.execute(values.flatten, lease_connection, allow_retry: true).first rescue TypeError raise ActiveRecord::StatementInvalid end diff --git a/activerecord/lib/active_record/internal_metadata.rb b/activerecord/lib/active_record/internal_metadata.rb index 1e1cccb44afc..ac2f3333b514 100644 --- a/activerecord/lib/active_record/internal_metadata.rb +++ b/activerecord/lib/active_record/internal_metadata.rb @@ -153,7 +153,7 @@ def update_entry(connection, key, new_value) def select_entry(connection, key) sm = Arel::SelectManager.new(arel_table) - sm.project(Arel::Nodes::SqlLiteral.new("*")) + sm.project(Arel::Nodes::SqlLiteral.new("*", retryable: true)) sm.where(arel_table[primary_key].eq(Arel::Nodes::BindParam.new(key))) sm.order(arel_table[primary_key].asc) sm.limit = 1 diff --git a/activerecord/lib/active_record/querying.rb b/activerecord/lib/active_record/querying.rb index 18a3f92c8e13..27d0603f3a9d 100644 --- a/activerecord/lib/active_record/querying.rb +++ b/activerecord/lib/active_record/querying.rb @@ -47,8 +47,8 @@ module Querying # # Note that building your own SQL query string from user input may expose your application to # injection attacks (https://guides.rubyonrails.org/security.html#sql-injection). - def find_by_sql(sql, binds = [], preparable: nil, &block) - _load_from_sql(_query_by_sql(sql, binds, preparable: preparable), &block) + def find_by_sql(sql, binds = [], preparable: nil, allow_retry: false, &block) + _load_from_sql(_query_by_sql(sql, binds, preparable: preparable, allow_retry: allow_retry), &block) end # Same as #find_by_sql but perform the query asynchronously and returns an ActiveRecord::Promise. @@ -58,8 +58,8 @@ def async_find_by_sql(sql, binds = [], preparable: nil, &block) end end - def _query_by_sql(sql, binds = [], preparable: nil, async: false) # :nodoc: - lease_connection.select_all(sanitize_sql(sql), "#{name} Load", binds, preparable: preparable, async: async) + def _query_by_sql(sql, binds = [], preparable: nil, async: false, allow_retry: false) # :nodoc: + lease_connection.select_all(sanitize_sql(sql), "#{name} Load", binds, preparable: preparable, async: async, allow_retry: allow_retry) end def _load_from_sql(result_set, &block) # :nodoc: diff --git a/activerecord/lib/active_record/relation/calculations.rb b/activerecord/lib/active_record/relation/calculations.rb index 620f99826a3a..86074e201773 100644 --- a/activerecord/lib/active_record/relation/calculations.rb +++ b/activerecord/lib/active_record/relation/calculations.rb @@ -446,7 +446,7 @@ def aggregate_column(column_name) return column_name if Arel::Expressions === column_name arel_column(column_name.to_s) do |name| - Arel.sql(column_name == :all ? "*" : name) + column_name == :all ? Arel.sql("*", retryable: true) : Arel.sql(name) end end @@ -643,7 +643,7 @@ def build_count_subquery(relation, column_name, distinct) relation.select_values = [ aggregate_column(column_name).as(column_alias) ] end - subquery_alias = Arel.sql("subquery_for_count") + subquery_alias = Arel.sql("subquery_for_count", retryable: true) select_value = operation_over_aggregate_column(column_alias, "count", false) relation.build_subquery(subquery_alias, select_value) diff --git a/activerecord/lib/active_record/relation/predicate_builder.rb b/activerecord/lib/active_record/relation/predicate_builder.rb index dd6cd573d8a9..878e74a42d65 100644 --- a/activerecord/lib/active_record/relation/predicate_builder.rb +++ b/activerecord/lib/active_record/relation/predicate_builder.rb @@ -28,9 +28,9 @@ def build_from_hash(attributes, &block) def self.references(attributes) attributes.each_with_object([]) do |(key, value), result| if value.is_a?(Hash) - result << Arel.sql(key) + result << Arel.sql(key, retryable: true) elsif (idx = key.rindex(".")) - result << Arel.sql(key[0, idx]) + result << Arel.sql(key[0, idx], retryable: true) end end end diff --git a/activerecord/lib/active_record/relation/query_methods.rb b/activerecord/lib/active_record/relation/query_methods.rb index 00425d93d375..6dddca5dda0a 100644 --- a/activerecord/lib/active_record/relation/query_methods.rb +++ b/activerecord/lib/active_record/relation/query_methods.rb @@ -2013,7 +2013,7 @@ def order_column(field) if attr_name == "count" && !group_values.empty? table[attr_name] else - Arel.sql(adapter_class.quote_table_name(attr_name)) + Arel.sql(adapter_class.quote_table_name(attr_name), retryable: true) end end end diff --git a/activerecord/lib/active_record/statement_cache.rb b/activerecord/lib/active_record/statement_cache.rb index 6c7359b77b8d..411a073a72c9 100644 --- a/activerecord/lib/active_record/statement_cache.rb +++ b/activerecord/lib/active_record/statement_cache.rb @@ -62,7 +62,7 @@ def sql_for(binds, connection) end class PartialQueryCollector - attr_accessor :preparable + attr_accessor :preparable, :retryable def initialize @parts = [] @@ -142,12 +142,12 @@ def initialize(query_builder, bind_map, klass) @klass = klass end - def execute(params, connection, &block) + def execute(params, connection, allow_retry: false, &block) bind_values = bind_map.bind params sql = query_builder.sql_for bind_values, connection - klass.find_by_sql(sql, bind_values, preparable: true, &block) + klass.find_by_sql(sql, bind_values, preparable: true, allow_retry: allow_retry, &block) rescue ::RangeError [] end diff --git a/activerecord/lib/arel.rb b/activerecord/lib/arel.rb index ecbde97e22dd..738e80df359f 100644 --- a/activerecord/lib/arel.rb +++ b/activerecord/lib/arel.rb @@ -45,16 +45,20 @@ module Arel # that this behavior only applies when bind value parameters are # supplied in the call; without them, the placeholder tokens have no # special meaning, and will be passed through to the query as-is. - def self.sql(sql_string, *positional_binds, **named_binds) + # + # The +:retryable+ option can be used to mark the SQL as safe to retry. + # Use this option only if the SQL is idempotent, as it could be executed + # more than once. + def self.sql(sql_string, *positional_binds, retryable: false, **named_binds) if positional_binds.empty? && named_binds.empty? - Arel::Nodes::SqlLiteral.new sql_string + Arel::Nodes::SqlLiteral.new(sql_string, retryable: retryable) else Arel::Nodes::BoundSqlLiteral.new sql_string, positional_binds, named_binds end end def self.star # :nodoc: - sql "*" + sql("*", retryable: true) end def self.arel_node?(value) # :nodoc: diff --git a/activerecord/lib/arel/alias_predication.rb b/activerecord/lib/arel/alias_predication.rb index 4abbbb7ef6de..1f7af26c25b4 100644 --- a/activerecord/lib/arel/alias_predication.rb +++ b/activerecord/lib/arel/alias_predication.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module AliasPredication def as(other) - Nodes::As.new self, Nodes::SqlLiteral.new(other) + Nodes::As.new self, Nodes::SqlLiteral.new(other, retryable: true) end end end diff --git a/activerecord/lib/arel/collectors/bind.rb b/activerecord/lib/arel/collectors/bind.rb index 6a96b1cb0e38..ee23051daf83 100644 --- a/activerecord/lib/arel/collectors/bind.rb +++ b/activerecord/lib/arel/collectors/bind.rb @@ -3,6 +3,8 @@ module Arel # :nodoc: all module Collectors class Bind + attr_accessor :retryable + def initialize @binds = [] end diff --git a/activerecord/lib/arel/collectors/composite.rb b/activerecord/lib/arel/collectors/composite.rb index 0f05dfbe548c..f31634c7e16c 100644 --- a/activerecord/lib/arel/collectors/composite.rb +++ b/activerecord/lib/arel/collectors/composite.rb @@ -4,12 +4,19 @@ module Arel # :nodoc: all module Collectors class Composite attr_accessor :preparable + attr_reader :retryable def initialize(left, right) @left = left @right = right end + def retryable=(retryable) + left.retryable = retryable + right.retryable = retryable + @retryable = retryable + end + def <<(str) left << str right << str diff --git a/activerecord/lib/arel/collectors/sql_string.rb b/activerecord/lib/arel/collectors/sql_string.rb index 8aa8958a1f85..b27eab4f6f2c 100644 --- a/activerecord/lib/arel/collectors/sql_string.rb +++ b/activerecord/lib/arel/collectors/sql_string.rb @@ -5,7 +5,7 @@ module Arel # :nodoc: all module Collectors class SQLString < PlainString - attr_accessor :preparable + attr_accessor :preparable, :retryable def initialize(*) super diff --git a/activerecord/lib/arel/collectors/substitute_binds.rb b/activerecord/lib/arel/collectors/substitute_binds.rb index 82315c75d321..020a53054a65 100644 --- a/activerecord/lib/arel/collectors/substitute_binds.rb +++ b/activerecord/lib/arel/collectors/substitute_binds.rb @@ -3,7 +3,7 @@ module Arel # :nodoc: all module Collectors class SubstituteBinds - attr_accessor :preparable + attr_accessor :preparable, :retryable def initialize(quoter, delegate_collector) @quoter = quoter diff --git a/activerecord/lib/arel/nodes/sql_literal.rb b/activerecord/lib/arel/nodes/sql_literal.rb index a6138b96bf01..f1862605d7fb 100644 --- a/activerecord/lib/arel/nodes/sql_literal.rb +++ b/activerecord/lib/arel/nodes/sql_literal.rb @@ -8,6 +8,13 @@ class SqlLiteral < String include Arel::AliasPredication include Arel::OrderPredications + attr_reader :retryable + + def initialize(string, retryable: false) + @retryable = retryable + super(string) + end + def encode_with(coder) coder.scalar = self.to_s end diff --git a/activerecord/lib/arel/select_manager.rb b/activerecord/lib/arel/select_manager.rb index dada3324eab4..2bdb6bc1110b 100644 --- a/activerecord/lib/arel/select_manager.rb +++ b/activerecord/lib/arel/select_manager.rb @@ -46,7 +46,7 @@ def exists end def as(other) - create_table_alias grouping(@ast), Nodes::SqlLiteral.new(other) + create_table_alias grouping(@ast), Nodes::SqlLiteral.new(other, retryable: true) end def lock(locking = Arel.sql("FOR UPDATE")) diff --git a/activerecord/lib/arel/visitors/mysql.rb b/activerecord/lib/arel/visitors/mysql.rb index 495c78ce3d04..937aa96da916 100644 --- a/activerecord/lib/arel/visitors/mysql.rb +++ b/activerecord/lib/arel/visitors/mysql.rb @@ -27,7 +27,7 @@ def visit_Arel_Nodes_SelectStatement(o, collector) end def visit_Arel_Nodes_SelectCore(o, collector) - o.froms ||= Arel.sql("DUAL") + o.froms ||= Arel.sql("DUAL", retryable: true) super end @@ -103,7 +103,7 @@ def build_subselect(key, o) Nodes::SelectStatement.new.tap do |stmt| core = stmt.cores.last core.froms = Nodes::Grouping.new(subselect).as("__active_record_temp") - core.projections = [Arel.sql(quote_column_name(key.name))] + core.projections = [Arel.sql(quote_column_name(key.name), retryable: true)] end end end diff --git a/activerecord/lib/arel/visitors/to_sql.rb b/activerecord/lib/arel/visitors/to_sql.rb index ff82d680c268..8c1586f9ff5e 100644 --- a/activerecord/lib/arel/visitors/to_sql.rb +++ b/activerecord/lib/arel/visitors/to_sql.rb @@ -20,6 +20,7 @@ def compile(node, collector = Arel::Collectors::SQLString.new) private def visit_Arel_Nodes_DeleteStatement(o, collector) + collector.retryable = false o = prepare_delete_statement(o) if has_join_sources?(o) @@ -37,6 +38,7 @@ def visit_Arel_Nodes_DeleteStatement(o, collector) end def visit_Arel_Nodes_UpdateStatement(o, collector) + collector.retryable = false o = prepare_update_statement(o) collector << "UPDATE " @@ -49,6 +51,7 @@ def visit_Arel_Nodes_UpdateStatement(o, collector) end def visit_Arel_Nodes_InsertStatement(o, collector) + collector.retryable = false collector << "INSERT INTO " collector = visit o.relation, collector @@ -381,6 +384,7 @@ def visit_Arel_Nodes_Group(o, collector) end def visit_Arel_Nodes_NamedFunction(o, collector) + collector.retryable = false collector << o.name collector << "(" collector << "DISTINCT " if o.distinct @@ -768,10 +772,12 @@ def visit_Arel_Nodes_BindParam(o, collector) def visit_Arel_Nodes_SqlLiteral(o, collector) collector.preparable = false + collector.retryable = o.retryable collector << o.to_s end def visit_Arel_Nodes_BoundSqlLiteral(o, collector) + collector.retryable = false bind_index = 0 new_bind = lambda do |value| diff --git a/activerecord/test/cases/adapter_test.rb b/activerecord/test/cases/adapter_test.rb index f6e6297dab2a..75c949c467c7 100644 --- a/activerecord/test/cases/adapter_test.rb +++ b/activerecord/test/cases/adapter_test.rb @@ -630,18 +630,78 @@ def teardown assert_predicate @connection, :active? end - test "querying after a failed query restores and succeeds" do + test "querying after a failed non-retryable query restores and succeeds" do Post.first # Connection verified (and prepared statement pool populated if enabled) remote_disconnect @connection assert_raises(ActiveRecord::ConnectionFailed) do - Post.first # Connection no longer verified after failed query + @connection.execute("INSERT INTO posts(title, body) VALUES ('foo', 'bar')") end assert Post.first # Verifying the connection causes a reconnect and the query succeeds + assert_predicate @connection, :active? + end + + test "idempotent SELECT queries are retried and result in a reconnect" do + Post.first + remote_disconnect @connection + + assert Post.first assert_predicate @connection, :active? + + remote_disconnect @connection + + assert Post.where(id: [1, 2]).first + assert_predicate @connection, :active? + end + + test "#find and #find_by queries with known attributes are retried and result in a reconnect" do + Post.first + + remote_disconnect @connection + + assert Post.find(1) + assert_predicate @connection, :active? + + remote_disconnect @connection + + assert Post.find_by(title: "Welcome to the weblog") + assert_predicate @connection, :active? + end + + test "queries containing SQL fragments are not retried" do + Post.first + + remote_disconnect @connection + + assert_raises(ActiveRecord::ConnectionFailed) { Post.where("1 = 1").to_a } + assert_not_predicate @connection, :active? + + remote_disconnect @connection + + assert_raises(ActiveRecord::ConnectionFailed) { Post.select("title AS custom_title").first } + assert_not_predicate @connection, :active? + + remote_disconnect @connection + + assert_raises(ActiveRecord::ConnectionFailed) { Post.find_by("updated_at < ?", 2.weeks.ago) } + assert_not_predicate @connection, :active? + end + + test "queries containing SQL functions are not retried" do + Post.first + + remote_disconnect @connection + + tags_count_attr = Post.arel_table[:tags_count] + abs_tags_count = Arel::Nodes::NamedFunction.new("ABS", [tags_count_attr]) + + assert_raises(ActiveRecord::ConnectionFailed) do + Post.where(abs_tags_count.eq(2)).first + end + assert_not_predicate @connection, :active? end test "transaction restores after remote disconnection" do @@ -779,6 +839,8 @@ def raw_transaction_open?(connection) def remote_disconnect(connection) case connection.adapter_name when "PostgreSQL" + # Connection was left in a bad state, need to reconnect to simulate fresh disconnect + connection.verify! if connection.instance_variable_get(:@raw_connection).status == ::PG::CONNECTION_BAD unless connection.instance_variable_get(:@raw_connection).transaction_status == ::PG::PQTRANS_INTRANS connection.instance_variable_get(:@raw_connection).async_exec("begin") end diff --git a/activerecord/test/cases/arel/collectors/composite_test.rb b/activerecord/test/cases/arel/collectors/composite_test.rb index fb910034d1b7..6fc3f712b86b 100644 --- a/activerecord/test/cases/arel/collectors/composite_test.rb +++ b/activerecord/test/cases/arel/collectors/composite_test.rb @@ -42,6 +42,16 @@ def test_composite_collector_performs_multiple_collections_at_once assert_equal 'SELECT FROM "users" WHERE "users"."age" = ? AND "users"."name" = ?', sql assert_equal ["hello2", "world3"], binds end + + def test_retryable_on_composite_collector_propagates + sql_collector = Collectors::SQLString.new + bind_collector = Collectors::Bind.new + collector = Collectors::Composite.new(sql_collector, bind_collector) + collector.retryable = true + + assert sql_collector.retryable + assert bind_collector.retryable + end end end end diff --git a/activerecord/test/cases/arel/visitors/to_sql_test.rb b/activerecord/test/cases/arel/visitors/to_sql_test.rb index 7f01ac50575f..b43e27561a99 100644 --- a/activerecord/test/cases/arel/visitors/to_sql_test.rb +++ b/activerecord/test/cases/arel/visitors/to_sql_test.rb @@ -69,6 +69,62 @@ def dispatch _(sql).must_be_like %{ omg(*) IS NULL } end + it "should mark collector as non-retryable when visiting named function" do + function = Nodes::NamedFunction.new("ABS", [@table]) + collector = Collectors::SQLString.new + @visitor.accept(function, collector) + + assert_equal false, collector.retryable + end + + it "should mark collector as non-retryable when visiting SQL literal" do + node = Nodes::SqlLiteral.new("COUNT(*)") + collector = Collectors::SQLString.new + @visitor.accept(node, collector) + + assert_equal false, collector.retryable + end + + it "should mark collector as retryable if SQL literal is marked as retryable" do + node = Nodes::SqlLiteral.new("COUNT(*)", retryable: true) + collector = Collectors::SQLString.new + @visitor.accept(node, collector) + + assert collector.retryable + end + + it "should mark collector as non-retryable when visiting bound SQL literal" do + node = Nodes::BoundSqlLiteral.new("id IN (?)", [[1, 2, 3]], {}) + collector = Collectors::SQLString.new + @visitor.accept(node, collector) + + assert_equal false, collector.retryable + end + + it "should mark collector as non-retryable when visiting insert statement node" do + statement = Arel::Nodes::InsertStatement.new(@table) + collector = Collectors::SQLString.new + @visitor.accept(statement, collector) + + assert_equal false, collector.retryable + end + + it "should mark collector as non-retryable when visiting update statement node" do + statement = Arel::Nodes::UpdateStatement.new(@table) + collector = Collectors::SQLString.new + @visitor.accept(statement, collector) + + assert_equal false, collector.retryable + end + + it "should mark collector as non-retryable when visiting delete statement node" do + statement = Arel::Nodes::DeleteStatement.new(@table) + collector = Collectors::SQLString.new + @visitor.accept(statement, collector) + + assert_equal false, collector.retryable + end + it "should visit built-in functions" do function = Nodes::Count.new([Arel.star]) assert_equal "COUNT(*)", compile(function)