Re: Error on failed COMMIT

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.

I'm not convinced of this. I doubt we actually have any real numbers?

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.
>
Clients can code around this as well for old servers. This is something
that is more palatable
if the server defines this behaviour.

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

I think his point is that the error is emitted before we actually do the
rollback and it could fail.

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

I guess the error has to be sent after the rollback completes.

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

Pretty sure I can code around this.

--
> Vik Fearing
>