diff options
author | Matthew Draper <matthew@trebex.net> | 2016-03-02 02:23:11 +1030 |
---|---|---|
committer | Matthew Draper <matthew@trebex.net> | 2016-03-02 02:23:11 +1030 |
commit | 833b14c8f8cae2f2ad2ea31669380b51e644c7db (patch) | |
tree | bab06923ab93aafa53818237788f26db7d44d105 /activerecord | |
parent | 664a13e6fb8281107da0da75e7cd91bba1425f76 (diff) | |
parent | 9e28f415f5aa52a3179d7cbef2594a0d0c03bcce (diff) | |
download | rails-833b14c8f8cae2f2ad2ea31669380b51e644c7db.tar.gz rails-833b14c8f8cae2f2ad2ea31669380b51e644c7db.tar.bz2 rails-833b14c8f8cae2f2ad2ea31669380b51e644c7db.zip |
Merge pull request #22170 from samphilipd/sam/properly_deallocate_prepared_statements_outside_of_transaction
Correctly deallocate prepared statements if we fail inside a transaction
Diffstat (limited to 'activerecord')
4 files changed, 135 insertions, 14 deletions
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb index 6ecdab6eb0..ca795cb1ad 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb @@ -188,7 +188,10 @@ module ActiveRecord transaction = begin_transaction options yield rescue Exception => error - rollback_transaction if transaction + if transaction + rollback_transaction + after_failure_actions(transaction, error) + end raise ensure unless error @@ -214,7 +217,16 @@ module ActiveRecord end private + NULL_TRANSACTION = NullTransaction.new + + # Deallocate invalidated prepared statements outside of the transaction + def after_failure_actions(transaction, error) + return unless transaction.is_a?(RealTransaction) + return unless error.is_a?(ActiveRecord::PreparedStatementCacheExpired) + @connection.clear_cache! + end + end end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index beaeef3c78..61c9628de3 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -598,25 +598,41 @@ module ActiveRecord @connection.exec_prepared(stmt_key, type_casted_binds) end rescue ActiveRecord::StatementInvalid => e - pgerror = e.cause + raise unless is_cached_plan_failure?(e) - # Get the PG code for the failure. Annoyingly, the code for - # prepared statements whose return value may have changed is - # FEATURE_NOT_SUPPORTED. Check here for more details: - # http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/cache/plancache.c#l573 - begin - code = pgerror.result.result_error_field(PGresult::PG_DIAG_SQLSTATE) - rescue - raise e - end - if FEATURE_NOT_SUPPORTED == code + # Nothing we can do if we are in a transaction because all commands + # will raise InFailedSQLTransaction + if in_transaction? + raise ActiveRecord::PreparedStatementCacheExpired.new(e.cause.message) + else + # outside of transactions we can simply flush this query and retry @statements.delete sql_key(sql) retry - else - raise e end end + # Annoyingly, the code for prepared statements whose return value may + # have changed is FEATURE_NOT_SUPPORTED. + # + # This covers various different error types so we need to do additional + # work to classify the exception definitively as a + # ActiveRecord::PreparedStatementCacheExpired + # + # Check here for more details: + # http://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/utils/cache/plancache.c#l573 + CACHED_PLAN_HEURISTIC = 'cached plan must not change result type'.freeze + def is_cached_plan_failure?(e) + pgerror = e.cause + code = pgerror.result.result_error_field(PGresult::PG_DIAG_SQLSTATE) + code == FEATURE_NOT_SUPPORTED && pgerror.message.include?(CACHED_PLAN_HEURISTIC) + rescue + false + end + + def in_transaction? + open_transactions > 0 + end + # Returns the statement identifier for the client side cache # of statements def sql_key(sql) diff --git a/activerecord/lib/active_record/errors.rb b/activerecord/lib/active_record/errors.rb index 87f32c042c..2ec9bf3d67 100644 --- a/activerecord/lib/active_record/errors.rb +++ b/activerecord/lib/active_record/errors.rb @@ -139,6 +139,11 @@ module ActiveRecord class NoDatabaseError < StatementInvalid end + # Raised when Postgres returns 'cached plan must not change result type' and + # we cannot retry gracefully (e.g. inside a transaction) + class PreparedStatementCacheExpired < StatementInvalid + end + # Raised on attempt to save stale record. Record is stale when it's being saved in another query after # instantiation, for example, when two users edit the same wiki page and one starts editing and saves # the page before the other. diff --git a/activerecord/test/cases/hot_compatibility_test.rb b/activerecord/test/cases/hot_compatibility_test.rb index 5ba9a1029a..9fc75b7377 100644 --- a/activerecord/test/cases/hot_compatibility_test.rb +++ b/activerecord/test/cases/hot_compatibility_test.rb @@ -1,7 +1,9 @@ require 'cases/helper' +require 'support/connection_helper' class HotCompatibilityTest < ActiveRecord::TestCase self.use_transactional_tests = false + include ConnectionHelper setup do @klass = Class.new(ActiveRecord::Base) do @@ -51,4 +53,90 @@ class HotCompatibilityTest < ActiveRecord::TestCase record.reload assert_equal 'bar', record.foo end + + if current_adapter?(:PostgreSQLAdapter) + test "cleans up after prepared statement failure in a transaction" do + with_two_connections do |original_connection, ddl_connection| + record = @klass.create! bar: 'bar' + + # prepare the reload statement in a transaction + @klass.transaction do + record.reload + end + + assert get_prepared_statement_cache(@klass.connection).any?, + "expected prepared statement cache to have something in it" + + # add a new column + ddl_connection.add_column :hot_compatibilities, :baz, :string + + assert_raise(ActiveRecord::PreparedStatementCacheExpired) do + @klass.transaction do + record.reload + end + end + + assert_empty get_prepared_statement_cache(@klass.connection), + "expected prepared statement cache to be empty but it wasn't" + end + end + + test "cleans up after prepared statement failure in nested transactions" do + with_two_connections do |original_connection, ddl_connection| + record = @klass.create! bar: 'bar' + + # prepare the reload statement in a transaction + @klass.transaction do + record.reload + end + + assert get_prepared_statement_cache(@klass.connection).any?, + "expected prepared statement cache to have something in it" + + # add a new column + ddl_connection.add_column :hot_compatibilities, :baz, :string + + assert_raise(ActiveRecord::PreparedStatementCacheExpired) do + @klass.transaction do + @klass.transaction do + @klass.transaction do + record.reload + end + end + end + end + + assert_empty get_prepared_statement_cache(@klass.connection), + "expected prepared statement cache to be empty but it wasn't" + end + end + end + + private + + def get_prepared_statement_cache(connection) + connection.instance_variable_get(:@statements) + .instance_variable_get(:@cache)[Process.pid] + end + + # Rails will automatically clear the prepared statements on the connection + # that runs the migration, so we use two connections to simulate what would + # actually happen on a production system; we'd have one connection running the + # migration from the rake task ("ddl_connection" here), and we'd have another + # connection in a web worker. + def with_two_connections + run_without_connection do |original_connection| + ActiveRecord::Base.establish_connection(original_connection.merge(pool_size: 2)) + begin + ddl_connection = ActiveRecord::Base.connection_pool.checkout + begin + yield original_connection, ddl_connection + ensure + ActiveRecord::Base.connection_pool.checkin ddl_connection + end + ensure + ActiveRecord::Base.clear_all_connections! + end + end + end end |