Correctly deallocate prepared statements if we fail inside a transaction#22170
Conversation
|
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. |
c4b3ec0 to
0016e1d
Compare
|
r? @sgrif |
e87a9c0 to
0d72268
Compare
|
@sgrif Going to look into the test failures on this one, will ping you when they are resolved. |
|
@sgrif OK tests passing and I cleaned up a few things, this is ready for review. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
|
I also considered adding a |
7f13991 to
c5cd2ac
Compare
- Addresses issue rails#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.
c5cd2ac to
50c5334
Compare
There was a problem hiding this comment.
if this is a Postgress specific error, I am not sure if we should have handled by all adapters.
There was a problem hiding this comment.
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.
…pared_statements_outside_of_transaction Correctly deallocate prepared statements if we fail inside a transaction
|
@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 I'm trying to determine where to rescue the exception - e.g. as far upstream as |
|
@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
endYou can call a retriable transaction like this: # Automatically retries in the event of ActiveRecord::PreparedStatementCacheExpired
ApplicationRecord.transaction do
...
endor # Automatically retries in the event of ActiveRecord::PreparedStatementCacheExpired
MyModel.transaction do
...
endNote 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 |
|
@mecampbellsoup here is a more detailed article on how to handle these errors. |
|
I am on Rails 5.1.6 and still are getting this error when underlying schema changes while the Rails process is running. Thanks! |
|
@marisveide there are a number of ways to check if a commit is present in a given branch using a version control system like git. They probably all are quicker for you to do than the time everyone on this issue takes to read your question. I'm just going to paste the link the merge commit github provides up on the page since it references this commit being present from Rails 5.0 beta through Rails 6 833b14c |
|
Hi, @bf4 ! We have long-running workers on Rails (with Sidekiq) and are getting this error in all workers which were running while DB schema was modified. Just in case, here is the stack trace from that exception: Any suggestions on how to proceed with seeking a fix to that? |
|
@marisveide this PR does not automatically fix that error, because the only way to fix it is to If you are sure that every rails transaction block in your codebase has no side effects you can follow the advice in this article. Otherwise, you should define a new |
Overview
Cached postgres prepared statements become invalidated if the schema
changes in a way that it affects the returned result.
Examples:
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
communicate any useful information about what has actually gone wrong.
deallocated - it stays alive for the length of the session taking up
memory on the postgres server.
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.