Re: Error on failed COMMIT

On Tue, 26 Jan 2021 at 05:05, 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.
>
> First off thanks for reviewing.

The problem is that ereport does not return for any level equal to or above
ERROR. This code required it to return so that it could continue processing
So after re-acquainting myself with the code I propose: Now we could use
"TRANSACTION_ERROR" instead of "USER_ERROR"
I'd like to comment more but I do not believe that elog.h is the place.
Suggestions ?

index 3c0e57621f..df79a2d6db 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -42,17 +42,19 @@
* WARNING
is for unexpected messages. */
#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 /* similar to ERROR, except
we don't want to
+ * exit the
current context. */
+#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 */

> Is this new message level something we need to allow setting for
> "client_min_messages" and "log_min_messages"?
>

Good question. I had not given that any thought.

Dave Cramer
www.postgres.rocks

>
>