Re: Error on failed COMMIT

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

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

There have been some arguments that the client can fix this easily.

Turns out it is not as easy as one might think.

If the client (in this case JDBC) uses conn.commit() then yes relatively
easy as we know that commit is being executed.

however if the client executes commit using direct SQL and possibly
multiplexes a number of commands we would have to parse the SQL to figure
out what is being sent. This could include a column named commit_date or a
comment with commit embedded in it. It really doesn't make sense to have a
full fledged PostgreSQL SQL parser in every client. This is something the
server does very well.

There has been another argument that we can simply check the transaction
state after we get the ReadyForQuery response, however this is set to IDLE
after the subsequent ROLLBACK so that doesn't work either.

Additionally in section 52.2.2 of the docs it states:

A frontend must be prepared to accept ErrorResponse and NoticeResponse
messages whenever it is expecting any other type of message. See also
Section 52.2.6 concerning messages that the backend might generate due to
outside events.

Recommended practice is to code frontends in a state-machine style that
will accept any message type at any time that it could make sense, rather
than wiring in assumptions about the exact sequence of messages.

Seems to me that this behaviour is already documented?

Dave Cramer
http://www.postgres.rocks

>