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

On Thu, 27 Feb 2020 at 07:44, Dave Cramer <davecramer@postgres.rocks> wrote:

On Wed, 26 Feb 2020 at 16:57, Vik Fearing <vik@postgresfriends.org> wrote:

On 26/02/2020 22:22, Tom Lane wrote:
> Dave Cramer <davecramer@postgres.rocks> writes:
>> OK, here is a patch that actually doesn't leave the transaction in a failed
>> state but emits the error and rolls back the transaction.
>
>> This is far from complete as it fails a number of tests  and does not cover
>> all of the possible paths.
>> But I'd like to know if this is strategy will be acceptable ?
>
> I really don't think that changing the server's behavior here is going to
> fly.  The people who are unhappy that we changed it are going to vastly
> outnumber the people who are happy.  Even the people who are happy are not
> going to find that their lives are improved all that much, because they'll
> still have to deal with old servers with the old behavior for the
> foreseeable future.

Dealing with old servers for a while is something that everyone is used to.

> Even granting that a behavioral incompatibility is acceptable, I'm not
> sure how a client is supposed to be sure that this "error" means that a
> rollback happened, as opposed to real errors that prevented any state
> change from occurring.

Because the error is a Class 40 — Transaction Rollback.

My original example was:

postgres=!# commit;
ERROR:  40P00: transaction cannot be committed
DETAIL:  First error was "42601: syntax error at or near "error"".


> (A trivial example of that is misspelling the
> COMMIT command; which I'll grant is unlikely in practice.  But there are
> less-trivial examples involving internal server malfunctions.)

Misspelling the COMMIT command is likely a syntax error, which is Class
42.  Can you give one of those less-trivial examples please?

> The only
> way to be sure you're out of the transaction is to check the transaction
> state that's sent along with ReadyForQuery ... but if you need to do
> that, it's not clear why we should change the server behavior at all.

How does this differ from the deferred constraint violation example you
provided early on in the thread?  That gave the error 23505 and
terminated the transaction.  If you run the same scenario with the
primary key immediate, you get the *exact same error* but the
transaction is *not* terminated!

I won't go so far as to suggest we change all COMMIT errors to Class 40
(as the spec says), but I'm thinking it very loudly.

> I also don't think that this scales to the case of subtransaction
> commit/rollback.  That should surely act the same, but your patch doesn't
> change it.

How does one commit a subtransaction?

> Lastly, introducing a new client-visible message level seems right out.
> That's a very fundamental protocol break, independently of all else.

Yeah, this seemed like a bad idea to me, too.

Ok, here is a much less obtrusive solution thanks to Vladimir.

Still had to mess with error levels since commit and chain needs the existing context to succeed.

After fixing up the tests only 1 still failing.