New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly deallocate prepared statements if we fail inside a transaction #22170

Merged
merged 2 commits into from Mar 1, 2016

Conversation

Projects
None yet
9 participants
@samphilipd
Contributor

samphilipd commented Nov 3, 2015

Overview

Cached postgres prepared statements become invalidated if the schema
changes in a way that it affects the returned result.

Examples:

  • adding or removing a column then doing a 'SELECT *'
  • removing the foo column then doing a 'SELECT bar.foo'

In normal operation this isn't a problem, we can rescue the error,
deallocate the prepared statement and re-issue the command.

However in PostgreSQL transactions, once any command fails, the
transaction becomes 'poisoned' and any subsequent commands will raise
InFailedSQLTransaction.

This includes DEALLOCATE statements, so the default deallocation
strategy instead of removing the cached prepared statement instead
raises InFailedSQLTransaction.

Why this is bad

  1. InFailedSQLTransaction is a fairly cryptic error and doesn't
    communicate any useful information about what has actually gone wrong.
  2. In the naive implementation the prepared statement never gets
    deallocated - it stays alive for the length of the session taking up
    memory on the postgres server.
  3. It is unsafe to retry the transaction because the bad prepared
    statement is still in the cache and we would see the exact same failure
    repeated.

Solution

If we are outside a transaction we can continue to handle these failures
gracefully in the usual way.

Inside a transaction instead of issuing a DEALLOCATE command that will
certainly fail, we now raise
ActiveRecord::PreparedStatementCacheExpired.

This can be handled further up the stack, notably inside
TransactionManager#within_new_transaction. Here we can make sure to
first rollback the transaction, then safely issue DEALLOCATE statements
to invalidate the rest of the cached prepared statements.

This also allows the user (or some gem) the opportunity to catch this error and
voluntarily retry the transaction if a schema change causes the prepared
statement cache to become invalidated.

Because the outdated statement has been deallocated, we can expect the
transaction to succeed on the second try.

@rails-bot

This comment has been minimized.

Show comment
Hide comment
@rails-bot

rails-bot Nov 3, 2015

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@schneems

This comment has been minimized.

Show comment
Hide comment
Member

schneems commented Nov 3, 2015

r? @sgrif

@rails-bot rails-bot assigned sgrif and unassigned schneems Nov 3, 2015

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Nov 3, 2015

Contributor

@sgrif Going to look into the test failures on this one, will ping you when they are resolved.

Contributor

samphilipd commented Nov 3, 2015

@sgrif Going to look into the test failures on this one, will ping you when they are resolved.

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Nov 3, 2015

Contributor

@sgrif OK tests passing and I cleaned up a few things, this is ready for review.

Contributor

samphilipd commented Nov 3, 2015

@sgrif OK tests passing and I cleaned up a few things, this is ready for review.

Show outdated Hide outdated activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
raise e
end
if FEATURE_NOT_SUPPORTED == code
raise unless is_cached_plan_failure?(e)

This comment has been minimized.

@HoyaBoya

HoyaBoya Nov 4, 2015

Out of curiosity, would this be raise e?

I'm tracking this PR for our own issues with prepared statements in transactions in Rails 4-2. This is really awesome work.

@HoyaBoya

HoyaBoya Nov 4, 2015

Out of curiosity, would this be raise e?

I'm tracking this PR for our own issues with prepared statements in transactions in Rails 4-2. This is really awesome work.

This comment has been minimized.

@samphilipd

samphilipd Nov 4, 2015

Contributor

In this particular case, raise e and raise have identical behavior. When called without an argument, raise simply re-raises the last exception, or a RuntimeError if there isn't one.

See http://ruby-doc.org/core-2.1.1/Kernel.html#method-i-raise .

@samphilipd

samphilipd Nov 4, 2015

Contributor

In this particular case, raise e and raise have identical behavior. When called without an argument, raise simply re-raises the last exception, or a RuntimeError if there isn't one.

See http://ruby-doc.org/core-2.1.1/Kernel.html#method-i-raise .

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd Nov 5, 2015

Contributor

I also considered adding a after_failed_transaction_hooks array to the transaction manager, but this turned out to be too general for just this one specific case.

Contributor

samphilipd commented Nov 5, 2015

I also considered adding a after_failed_transaction_hooks array to the transaction manager, but this turned out to be too general for just this one specific case.

Sam Davies added some commits Nov 2, 2015

Sam Davies
Correctly deallocate prepared statements if we fail inside a transaction
- Addresses issue #12330

Overview
========

Cached postgres prepared statements become invalidated if the schema
changes in a way that it affects the returned result.

Examples:
- adding or removing a column then doing a 'SELECT *'
- removing the foo column  then doing a 'SELECT bar.foo'

In normal operation this isn't a problem, we can rescue the error,
deallocate the prepared statement and re-issue the command.

However in PostgreSQL transactions, once any command fails, the
transaction becomes 'poisoned' and any subsequent commands will raise
InFailedSQLTransaction.

This includes DEALLOCATE statements, so the default deallocation
strategy instead of removing the cached prepared statement instead
raises InFailedSQLTransaction.

Why this is bad
===============

1. InFailedSQLTransaction is a fairly cryptic error and doesn't
communicate any useful information about what has actually gone wrong.
2. In the naive implementation the prepared statement never gets
deallocated - it stays alive for the length of the session taking up
memory on the postgres server.
3. It is unsafe to retry the transaction because the bad prepared
statement is still in the cache and we would see the exact same failure
repeated.

Solution
========

If we are outside a transaction we can continue to handle these failures
gracefully in the usual way.

Inside a transaction instead of issuing a DEALLOCATE command that will
certainly fail, we now raise
ActiveRecord::PreparedStatementCacheExpired.

This can be handled further up the stack, notably inside
TransactionManager#within_new_transaction. Here we can make sure to
first rollback the transaction, then safely issue DEALLOCATE statements
to invalidate the rest of the cached prepared statements.

This also allows the user (or some gem) the opportunity to catch this error and
voluntarily retry the transaction if a schema change causes the prepared
statement cache to become invalidated.

Because the outdated statement has been deallocated, we can expect the
transaction to succeed on the second try.

@samphilipd samphilipd changed the title from Correctly deallocate prepared statements if we are inside a transaction to Correctly deallocate prepared statements if we fail inside a transaction Nov 5, 2015

# 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)

This comment has been minimized.

@arthurnn

arthurnn Dec 5, 2015

Member

if this is a Postgress specific error, I am not sure if we should have handled by all adapters.

@arthurnn

arthurnn Dec 5, 2015

Member

if this is a Postgress specific error, I am not sure if we should have handled by all adapters.

This comment has been minimized.

@samphilipd

samphilipd Dec 8, 2015

Contributor

I agree in principle, except I couldn't find any place to put adapter-specific transaction behavior.

This method is the 'lowest' possible point in which we can access something outside of the transaction. While it's not ideal, I can't see a less invasive way of implementing this.

@samphilipd

samphilipd Dec 8, 2015

Contributor

I agree in principle, except I couldn't find any place to put adapter-specific transaction behavior.

This method is the 'lowest' possible point in which we can access something outside of the transaction. While it's not ideal, I can't see a less invasive way of implementing this.

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)

This comment has been minimized.

@matthewd

matthewd Dec 5, 2015

Member

Should we just kill the whole cache here too?

We got lucky on this query, but assuming we have other plans referencing the invalidated relation, the next query we hit could easily be inside a transaction.

@matthewd

matthewd Dec 5, 2015

Member

Should we just kill the whole cache here too?

We got lucky on this query, but assuming we have other plans referencing the invalidated relation, the next query we hit could easily be inside a transaction.

This comment has been minimized.

@samphilipd

samphilipd Dec 8, 2015

Contributor

@matthewd I deliberately left the behavior outside of a transaction exactly as it was before to make this change as non-invasive as possible whilst still solving the core problem.

I'm not an expert here, the person to ask would be @tenderlove who wrote this original case.

I think that invalidating the entire cache in all cases would be a safe response. The only time this Postgres exception is likely to be raised is on a schema change. These are rare enough that the performance hit of flushing the entire prepared statement cache is a minor issue.

@samphilipd

samphilipd Dec 8, 2015

Contributor

@matthewd I deliberately left the behavior outside of a transaction exactly as it was before to make this change as non-invasive as possible whilst still solving the core problem.

I'm not an expert here, the person to ask would be @tenderlove who wrote this original case.

I think that invalidating the entire cache in all cases would be a safe response. The only time this Postgres exception is likely to be raised is on a schema change. These are rare enough that the performance hit of flushing the entire prepared statement cache is a minor issue.

@matthewd matthewd added this to the 5.0.0 milestone Feb 14, 2016

@matthewd matthewd assigned matthewd and unassigned sgrif Feb 14, 2016

matthewd added a commit that referenced this pull request Mar 1, 2016

Merge pull request #22170 from samphilipd/sam/properly_deallocate_pre…
…pared_statements_outside_of_transaction

 Correctly deallocate prepared statements if we fail inside a transaction

@matthewd matthewd merged commit 833b14c into rails:master Mar 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@samphilipd samphilipd deleted the samphilipd:sam/properly_deallocate_prepared_statements_outside_of_transaction branch Apr 26, 2016

@mecampbellsoup

This comment has been minimized.

Show comment
Hide comment
@mecampbellsoup

mecampbellsoup May 5, 2017

Contributor

@samphilipd great work on this PR and the issue it originated from - saves us a lot of time and headache, shout out to you! 😄

I wanted to ask: do you have any patterns you'd recommend for handling the ActiveRecord::PreparedStatementCacheExpired exceptions which are now raised?

I'm trying to determine where to rescue the exception - e.g. as far upstream as ApplicationController? Maybe in ApplicationRecord callback/hook so that all the models communicating with the DB can be programmed to auto-retry the Postgres DB command when they see this exception?

Contributor

mecampbellsoup commented May 5, 2017

@samphilipd great work on this PR and the issue it originated from - saves us a lot of time and headache, shout out to you! 😄

I wanted to ask: do you have any patterns you'd recommend for handling the ActiveRecord::PreparedStatementCacheExpired exceptions which are now raised?

I'm trying to determine where to rescue the exception - e.g. as far upstream as ApplicationController? Maybe in ApplicationRecord callback/hook so that all the models communicating with the DB can be programmed to auto-retry the Postgres DB command when they see this exception?

@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd May 5, 2017

Contributor

@mecampbellsoup you could do something like this:

# Make all transactions for all records automatically retriable in the event of
# cache failure
class ApplicationRecord
  class << self
    # Retry automatically on ActiveRecord::PreparedStatementCacheExpired.
    #
    # Do not use this for transactions with side-effects unless it is acceptable
    # for these side-effects to occasionally happen twice
    def transaction(*args, &block)
      retried ||= false
      super
    rescue ActiveRecord::PreparedStatementCacheExpired
      if retried
        raise
      else
        retried = true
        retry
      end
    end
  end
end

You can call a retriable transaction like this:

# Automatically retries in the event of ActiveRecord::PreparedStatementCacheExpired
ApplicationRecord.transaction do
  ...
end

or

# Automatically retries in the event of ActiveRecord::PreparedStatementCacheExpired
MyModel.transaction do
  ...
end

Note that if you are sending emails, POSTing to an API or doing other such things that interact with the outside world inside your transactions, this could result in some of those things occasionally happening twice.

If you have a transaction with side-effects and would prefer to raise an error rather than auto-retry in the event of this error, you can call the original like this:

# Raises instead of retries on ActiveRecord::PreparedStatementCacheExpired
ActiveRecord::Base.transaction do
  ...
end
Contributor

samphilipd commented May 5, 2017

@mecampbellsoup you could do something like this:

# Make all transactions for all records automatically retriable in the event of
# cache failure
class ApplicationRecord
  class << self
    # Retry automatically on ActiveRecord::PreparedStatementCacheExpired.
    #
    # Do not use this for transactions with side-effects unless it is acceptable
    # for these side-effects to occasionally happen twice
    def transaction(*args, &block)
      retried ||= false
      super
    rescue ActiveRecord::PreparedStatementCacheExpired
      if retried
        raise
      else
        retried = true
        retry
      end
    end
  end
end

You can call a retriable transaction like this:

# Automatically retries in the event of ActiveRecord::PreparedStatementCacheExpired
ApplicationRecord.transaction do
  ...
end

or

# Automatically retries in the event of ActiveRecord::PreparedStatementCacheExpired
MyModel.transaction do
  ...
end

Note that if you are sending emails, POSTing to an API or doing other such things that interact with the outside world inside your transactions, this could result in some of those things occasionally happening twice.

If you have a transaction with side-effects and would prefer to raise an error rather than auto-retry in the event of this error, you can call the original like this:

# Raises instead of retries on ActiveRecord::PreparedStatementCacheExpired
ActiveRecord::Base.transaction do
  ...
end
@samphilipd

This comment has been minimized.

Show comment
Hide comment
@samphilipd

samphilipd May 6, 2017

Contributor

@mecampbellsoup here is a more detailed article on how to handle these errors.

Contributor

samphilipd commented May 6, 2017

@mecampbellsoup here is a more detailed article on how to handle these errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment