On Tue, Jan 26, 2021 at 7:06 PM Laurenz Albe <laurenz.a...@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, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
