On Thu, 27 Feb 2020 at 07:44, Dave Cramer <davecramer@postgres.rocks> wrote:
> > > > On Wed, 26 Feb 2020 at 16:57, Vik Fearing <v...@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. Dave Cramer http://www.postgres.rocks
throwerror2.patch
Description: Binary data
