-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix spurious aborts when retrying transactions #34
Conversation
Transactions that enter an error state must be aborted manually by issuing a "ROLLBACK". However, if the transaction error happened during a "COMMIT" then the rollback happens automatically. Issuing a "ROLLBACK" at this point causes PostgreSQL to issue a "WARNING: There is no transaction in progress". This warning can have much worse causes (e.g. you "COMMIT" but never began a transaction). This change makes the transaction retrying logic never cause PostgreSQL to issue this warning making it a more useful warning for detecting real bugs.
04bb48f
to
184c74f
Compare
Is there some easy to reproduce error case for |
As for reproduction, we would need to produce a serialization error which can be pretty tricky to do. If you run two identical mutating transactions in SERIALIZABLE isolation in parallel at high frequency you might get one of these errors within a few seconds. But even then, you'd need to instrument this code to determine if you actually hit it. |
Of course, I confirmed that this fixed the error in my specific use case. But that's not nearly as reassuring. Are there tests for this code at all? |
I think there aren't, but that's not to reason to add them. I'd be happy with any About instrumentation: good point, yet I'll be fine if that test doesn't work before this patch, but works afterwards. |
The main thing this fixes is a warning printed to stdout by [presumably] libpq. To make a test fail prior to this change you'd have to monitor stdout and look for the warning! |
One way to test this would be to use deferred constraints to get an exception at |
CREATE TABLE tmp (value INT NOT NULL UNIQUE DEFERRABLE);
BEGIN;
SET CONSTRAINTS ALL DEFERRED;
INSERT INTO tmp (value) VALUES (1);
INSERT INTO tmp (value) VALUES (1);
COMMIT; This will throw an error on the |
@3noch thanks for an example! |
@@ -157,20 +157,19 @@ withTransactionModeRetry :: TransactionMode -> (SqlError -> Bool) -> Connection | |||
withTransactionModeRetry mode shouldRetry conn act = | |||
mask $ \restore -> | |||
retryLoop $ E.try $ do | |||
a <- restore act | |||
a <- restore act `E.onException` rollback_ conn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, rollback_
is not exported and it's also suspicious. It catches exceptions thrown when issuing the rollback. Why is that a good idea?
Merged as part of #54 |
Thanks! |
Transactions that enter an error state must be aborted manually by
issuing a "ROLLBACK". However, if the transaction error happened during
a "COMMIT" then the rollback happens automatically. Issuing a "ROLLBACK"
at this point causes PostgreSQL to issue a "WARNING: There is no
transaction in progress". This warning can have much worse causes (e.g.
you "COMMIT" but never began a transaction). This change makes the
transaction retrying logic never cause PostgreSQL to issue this warning
making it a more useful warning for detecting real bugs.