Re: Error on failed COMMIT - Mailing list pgsql-hackers

On 11/02/2020 23:35, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> There is a current discussion off-list about what should happen when a
>> COMMIT is issued for a transaction that cannot be committed for whatever
>> reason.  PostgreSQL returns ROLLBACK as command tag but otherwise succeeds.
> 
>> It seems like [ trying to commit a failed transaction ]
>> should actually produce something like this:
> 
>> postgres=!# commit;
>> ERROR:  40P00: transaction cannot be committed
>> DETAIL:  First error was "42601: syntax error at or near "error""
> 
> So I assume you're imagining that that would leave us still in
> transaction-aborted state, and the session is basically dead in
> the water until the user thinks to issue ROLLBACK instead?

Actually, I was imagining that it would end the transaction as it does
today, just with an error code.

This is backed up by General Rule 9 which says "The current
SQL-transaction is terminated."

>> Is this reading correct?
> 
> Probably it is, according to the letter of the SQL spec, but I'm
> afraid that changing this behavior now would provoke lots of hate
> and few compliments.  An application that's doing the spec-compliant
> thing and issuing ROLLBACK isn't going to be affected, but apps that
> are relying on the existing behavior are going to be badly broken.

I figured that was likely.  I'm hoping to at least get a documentation
patch out of this thread, though.

> A related problem is what happens if you're in a perfectly-fine
> transaction and the commit itself fails, e.g.,
> 
> regression=# create table tt (f1 int primary key deferrable initially deferred);
> CREATE TABLE
> regression=# begin;
> BEGIN
> regression=# insert into tt values (1);
> INSERT 0 1
> regression=# insert into tt values (1);
> INSERT 0 1
> regression=# commit;
> ERROR:  duplicate key value violates unique constraint "tt_pkey"
> DETAIL:  Key (f1)=(1) already exists.
> 
> At this point PG considers that you're out of the transaction:
> 
> regression=# rollback;
> WARNING:  there is no transaction in progress
> ROLLBACK
> 
> but I bet the spec doesn't.  So if we change that, again we break
> applications that work today.

I would argue that the this example is entirely compliant and consistent
with my original question (except that it gives a class 23 instead of a
class 40).

> Meanwhile, an app that is doing it
> the spec-compliant way will issue a ROLLBACK that we consider
> useless, so currently that draws an ignorable WARNING and all is
> well.  So here also, the prospects for making more people happy
> than we make unhappy seem pretty grim.

I'm not entirely sure what should happen with a free-range ROLLBACK. (I
*think* it says it should error with "2D000 invalid transaction
termination" but it's a little confusing to me.)

> (Maybe there's a case for downgrading the WARNING to NOTICE, though?)

Maybe.  But I think its match (a double START TRANSACTION) should remain
a warning if we do change this.

> (Don't even *think* of suggesting that having a GUC to change
> this behavior would be appropriate.  The long-ago fiasco around
> autocommit showed us the hazards of letting GUCs affect such
> fundamental behavior.)

That thought never crossed my mind.

> Speaking of autocommit, I wonder how that would interact with
> this...

I don't see how it would be any different.
-- 
Vik Fearing