Re: Error on failed COMMIT

On Thu, 27 Feb 2020 at 07:44, Dave Cramer <davecramer(at)postgres(dot)rocks> wrote:

>
>
>
> On Wed, 26 Feb 2020 at 16:57, Vik Fearing <vik(at)postgresfriends(dot)org> wrote:
>
>> On 26/02/2020 22:22, Tom Lane wrote:
>> > Dave Cramer <davecramer(at)postgres(dot)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.

Dave Cramer
http://www.postgres.rocks