Fix spurious aborts when retrying transactions

[3noch]

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.

[phadej]

Is there some easy to reproduce error case for COMMIT, so we can write a test?

[3noch]

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.

[3noch]

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?

[phadej]

I think there aren't, but that's not to reason to add them.

I'd be happy with any withTransactionModeRetry tests, during the transaction generatingSqlError is easy, isn't it? I'm not asking you to write that though: If you could sketch how the test for SqlError in COMMIT would work (what kind of queries to run, etc.), I can try to make it into a test. I hope that on actually multi-core machine (which I luckily have) situation will happen multiple times.

About instrumentation: good point, yet I'll be fine if that test doesn't work before this patch, but works afterwards.

[3noch]

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!

[3noch]

One way to test this would be to use deferred constraints to get an exception at commit. shouldRetry could act as some sort of instrumentation if you're willing to use unsafePerformIO.

[3noch]

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 COMMIT.

[phadej]

@3noch thanks for an example!

[3noch]

BTW, rollback_ is not exported and it's also suspicious. It catches exceptions thrown when issuing the rollback. Why is that a good idea?

[phadej]

Merged as part of #54

[3noch]

Thanks!