Re: Error on failed COMMIT

On Tue, 26 Jan 2021 at 06:59, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:

> On Tue, Jan 26, 2021 at 7:06 PM Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at>
> wrote:
> >
> > On Mon, 2021-01-25 at 11:29 -0500, Dave Cramer wrote:
> > > Rebased against head
> > >
> > > Here's my summary of the long thread above.
> > >
> > > This change is in keeping with the SQL spec.
> > >
> > > There is an argument (Tom) that says that this will annoy more people
> than it will please.
> > > I presume this is due to the fact that libpq behaviour will change.
> > >
> > > As the author of the JDBC driver, and I believe I speak for other
> driver (NPGSQL for one)
> > > authors as well that have implemented the protocol I would argue that
> the current behaviour
> > > is more annoying.
> > >
> > > We currently have to keep state and determine if COMMIT actually
> failed or it ROLLED BACK.
> > > There are a number of async drivers that would also benefit from not
> having to keep state
> > > in the session.
> >
> > I think this change makes sense, but I think everybody agrees that it
> does as it
> > makes PostgreSQL more standard compliant.
> >
> > About the fear that it will break user's applications:
> >
> > I think that the breakage will be minimal. All that will change is that
> COMMIT of
> > an aborted transaction raises an error.
> >
> > Applications that catch an error in a transaction and roll back will not
> > be affected. What will be affected are applications that do *not* check
> for
> > errors in statements in a transaction, but check for errors in the
> COMMIT.
> > I think that doesn't happen often.
> >
> > I agree that some people will be hurt, but I don't think it will be a
> major problem.
> >
> > The patch applies and passes regression tests.
> >
> > I wonder about the introduction of the new USER_ERROR level:
> >
> > #define WARNING_CLIENT_ONLY 20 /* Warnings to be sent to client as
> usual, but
> > * never to the server log. */
> > -#define ERROR 21 /* user error - abort transaction;
> return to
> > +#define USER_ERROR 21
> > +#define ERROR 22 /* user error - abort transaction;
> return to
> > * known state */
> > /* Save ERROR value in PGERROR so it can be restored when Win32 includes
> > * modify it. We have to use a constant rather than ERROR because
> macros
> > * are expanded only when referenced outside macros.
> > */
> > #ifdef WIN32
> > -#define PGERROR 21
> > +#define PGERROR 22
> > #endif
> > -#define FATAL 22 /* fatal error - abort process */
> > -#define PANIC 23 /* take down the other backends with me
> */
> > +#define FATAL 23 /* fatal error - abort process */
> > +#define PANIC 24 /* take down the other backends with me
> */
> >
> > I see that without that, COMMIT AND CHAIN does not behave correctly,
> > since the respective regression tests fail.
> >
> > But I don't understand why. I think that this needs some more comments
> to
> > make this clear.
>
> While testing the patch I realized that the client gets an
> acknowledgment of COMMIT command completed successfully from
> PostgreSQL server (i.g., PQgetResult() returns PGRES_COMMAND_OK) even
> if the server raises an USER_ERROR level error. I think the command
> should be failed. Because otherwise, the drivers need to throw an
> exception by re-interpreting the results even in a case where the
> command is completed successfully.
>
> Regards,
>

Interesting. Thanks for looking at this. I'm curious what we return now
when we return rollback instead

Dave

>
> --
> Masahiko Sawada
> EDB: https://www.enterprisedb.com/
>