On Tue, 26 Jan 2021 at 06:59, Masahiko Sawada <sawada.m...@gmail.com> wrote:
> 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, > 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/ >
