Re: Error on failed COMMIT - Mailing list pgsql-hackers
From | Dave Cramer |
---|---|
Subject | Re: Error on failed COMMIT |
Date | |
Msg-id | CADK3HHK2hUaOez-=Ua1Ti=y+WaN+4q=ikuvvomGCA9bERVu0TA@mail.gmail.com Whole thread Raw |
In response to | Re: Error on failed COMMIT (Masahiko Sawada <sawada.mshk@gmail.com>) |
List | pgsql-hackers |
On Tue, 26 Jan 2021 at 06:59, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
On Tue, Jan 26, 2021 at 7:06 PM Laurenz Albe <laurenz.albe@cybertec.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/
pgsql-hackers by date: