Thread: Error on failed COMMIT

There is a current discussion off-list about what should happen when a
COMMIT is issued for a transaction that cannot be committed for whatever
reason.  PostgreSQL returns ROLLBACK as command tag but otherwise succeeds.

Here is an excerpt of Section 17.7 <commit statement> that I feel is
relevant:

<>
6) Case:

a) If any enforced constraint is not satisfied, then any changes to
SQL-data or schemas that were made by the current SQL-transaction are
canceled and an exception condition is raised: transaction rollback —
integrity constraint violation.

b) If any other error preventing commitment of the SQL-transaction has
occurred, then any changes to SQL-data or schemas that were made by the
current SQL-transaction are canceled and an exception condition is
raised: transaction rollback with an implementation-defined subclass value.

c) Otherwise, any changes to SQL-data or schemas that were made by the
current SQL-transaction are eligible to be perceived by all concurrent
and subsequent SQL-transactions.
</>

It seems like this:

postgres=# \set VERBOSITY verbose
postgres=# begin;
BEGIN
postgres=*# error;
ERROR:  42601: syntax error at or near "error"
LINE 1: error;
        ^
LOCATION:  scanner_yyerror, scan.l:1150
postgres=!# commit;
ROLLBACK

should actually produce something like this:

postgres=!# commit;
ERROR:  40P00: transaction cannot be committed
DETAIL:  First error was "42601: syntax error at or near "error""

Is this reading correct?
If so, is this something we should fix?
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> There is a current discussion off-list about what should happen when a
> COMMIT is issued for a transaction that cannot be committed for whatever
> reason.  PostgreSQL returns ROLLBACK as command tag but otherwise succeeds.

> It seems like [ trying to commit a failed transaction ]
> should actually produce something like this:

> postgres=!# commit;
> ERROR:  40P00: transaction cannot be committed
> DETAIL:  First error was "42601: syntax error at or near "error""

So I assume you're imagining that that would leave us still in
transaction-aborted state, and the session is basically dead in
the water until the user thinks to issue ROLLBACK instead?

> Is this reading correct?

Probably it is, according to the letter of the SQL spec, but I'm
afraid that changing this behavior now would provoke lots of hate
and few compliments.  An application that's doing the spec-compliant
thing and issuing ROLLBACK isn't going to be affected, but apps that
are relying on the existing behavior are going to be badly broken.

A related problem is what happens if you're in a perfectly-fine
transaction and the commit itself fails, e.g.,

regression=# create table tt (f1 int primary key deferrable initially deferred);
CREATE TABLE
regression=# begin;
BEGIN
regression=# insert into tt values (1);
INSERT 0 1
regression=# insert into tt values (1);
INSERT 0 1
regression=# commit;
ERROR:  duplicate key value violates unique constraint "tt_pkey"
DETAIL:  Key (f1)=(1) already exists.

At this point PG considers that you're out of the transaction:

regression=# rollback;
WARNING:  there is no transaction in progress
ROLLBACK

but I bet the spec doesn't.  So if we change that, again we break
applications that work today.  Meanwhile, an app that is doing it
the spec-compliant way will issue a ROLLBACK that we consider
useless, so currently that draws an ignorable WARNING and all is
well.  So here also, the prospects for making more people happy
than we make unhappy seem pretty grim.  (Maybe there's a case
for downgrading the WARNING to NOTICE, though?)

(Don't even *think* of suggesting that having a GUC to change
this behavior would be appropriate.  The long-ago fiasco around
autocommit showed us the hazards of letting GUCs affect such
fundamental behavior.)

Speaking of autocommit, I wonder how that would interact with
this...

            regards, tom lane

On 11/02/2020 23:35, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> There is a current discussion off-list about what should happen when a
>> COMMIT is issued for a transaction that cannot be committed for whatever
>> reason.  PostgreSQL returns ROLLBACK as command tag but otherwise succeeds.
> 
>> It seems like [ trying to commit a failed transaction ]
>> should actually produce something like this:
> 
>> postgres=!# commit;
>> ERROR:  40P00: transaction cannot be committed
>> DETAIL:  First error was "42601: syntax error at or near "error""
> 
> So I assume you're imagining that that would leave us still in
> transaction-aborted state, and the session is basically dead in
> the water until the user thinks to issue ROLLBACK instead?

Actually, I was imagining that it would end the transaction as it does
today, just with an error code.

This is backed up by General Rule 9 which says "The current
SQL-transaction is terminated."

>> Is this reading correct?
> 
> Probably it is, according to the letter of the SQL spec, but I'm
> afraid that changing this behavior now would provoke lots of hate
> and few compliments.  An application that's doing the spec-compliant
> thing and issuing ROLLBACK isn't going to be affected, but apps that
> are relying on the existing behavior are going to be badly broken.

I figured that was likely.  I'm hoping to at least get a documentation
patch out of this thread, though.

> A related problem is what happens if you're in a perfectly-fine
> transaction and the commit itself fails, e.g.,
> 
> regression=# create table tt (f1 int primary key deferrable initially deferred);
> CREATE TABLE
> regression=# begin;
> BEGIN
> regression=# insert into tt values (1);
> INSERT 0 1
> regression=# insert into tt values (1);
> INSERT 0 1
> regression=# commit;
> ERROR:  duplicate key value violates unique constraint "tt_pkey"
> DETAIL:  Key (f1)=(1) already exists.
> 
> At this point PG considers that you're out of the transaction:
> 
> regression=# rollback;
> WARNING:  there is no transaction in progress
> ROLLBACK
> 
> but I bet the spec doesn't.  So if we change that, again we break
> applications that work today.

I would argue that the this example is entirely compliant and consistent
with my original question (except that it gives a class 23 instead of a
class 40).

> Meanwhile, an app that is doing it
> the spec-compliant way will issue a ROLLBACK that we consider
> useless, so currently that draws an ignorable WARNING and all is
> well.  So here also, the prospects for making more people happy
> than we make unhappy seem pretty grim.

I'm not entirely sure what should happen with a free-range ROLLBACK. (I
*think* it says it should error with "2D000 invalid transaction
termination" but it's a little confusing to me.)

> (Maybe there's a case for downgrading the WARNING to NOTICE, though?)

Maybe.  But I think its match (a double START TRANSACTION) should remain
a warning if we do change this.

> (Don't even *think* of suggesting that having a GUC to change
> this behavior would be appropriate.  The long-ago fiasco around
> autocommit showed us the hazards of letting GUCs affect such
> fundamental behavior.)

That thought never crossed my mind.

> Speaking of autocommit, I wonder how that would interact with
> this...

I don't see how it would be any different.
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> On 11/02/2020 23:35, Tom Lane wrote:
>> So I assume you're imagining that that would leave us still in
>> transaction-aborted state, and the session is basically dead in
>> the water until the user thinks to issue ROLLBACK instead?

> Actually, I was imagining that it would end the transaction as it does
> today, just with an error code.
> This is backed up by General Rule 9 which says "The current
> SQL-transaction is terminated."

Hm ... that would be sensible, but I'm not entirely convinced.  There
are several preceding rules that say that an exception condition is
raised, and normally you can stop reading at that point; nothing else
is going to happen.  If COMMIT acts specially in this respect, they
ought to say so.

In any case, while this interpretation might change the calculus a bit,
I think we still end up concluding that altering this behavior has more
downside than upside.

            regards, tom lane

Am 12.02.2020 um 00:27 schrieb Tom Lane:
> Vik Fearing <vik@postgresfriends.org> writes:
>> Actually, I was imagining that it would end the transaction as it does
>> today, just with an error code.
>> This is backed up by General Rule 9 which says "The current
>> SQL-transaction is terminated."
> Hm ... that would be sensible, but I'm not entirely convinced.  There
> are several preceding rules that say that an exception condition is
> raised, and normally you can stop reading at that point; nothing else
> is going to happen.  If COMMIT acts specially in this respect, they
> ought to say so.
>
> In any case, while this interpretation might change the calculus a bit,
> I think we still end up concluding that altering this behavior has more
> downside than upside.

Let me illustrate this issue from an application (framework) developer's 
perspective:

When an application interacts with a database, it must be clearly 
possible to determine, whether a commit actually succeeded (and made all 
changes persistent), or the commit failed for any reason (and all of the 
changes have been rolled back). If a commit succeeds, an application 
must be allowed to assume that all changes it made in the preceeding 
transaction are made persistent and it is valid to update its internal 
state (e.g. caches) to the values updated in the transaction. This must 
be possible, even if the transaction is constructed collaboratively by 
multipe independent layers of the application (e.g. a framework and an 
application layer). Unfortunately, this seems not to be possible with 
the current implementation - at least not with default settings:

Assume the application is written in Java and sees Postgres through the 
JDBC driver:

composeTransaction() {
    Connection con = getConnection(); // implicitly "begin"
    try {
       insertFrameworkLevelState(con);
       insertApplicationLevelState(con);
       con.commit();
       publishNewState();
    } catch (Throwable ex) {
       con.rollback();
    }
}

With the current implementation, it is possible, that the control flow 
reaches "publishNewState()" without the changes done in 
"insertFrameworkLevelState()" have been made persistent - without the 
framework-level code (which is everything except 
"insertApplicationLevelState()") being able to detect the problem (e.g. 
if "insertApplicationLevelState()" tries add a null into a non-null 
column catching the exception or any other application-level error that 
is not properly handled through safepoints).

 From a framework's perspective, this behavior is absolutely 
unacceptable. Here, the framework-level code sees a database that 
commits successfully but does not make its changes persistent. 
Therefore, I don't think that altering this behavior has more downside 
than upside.

Best regards

Bernhard

On Thu, Feb 13, 2020 at 2:38 AM Haumacher, Bernhard <haui@haumacher.de> wrote:
> Assume the application is written in Java and sees Postgres through the
> JDBC driver:
>
> composeTransaction() {
>     Connection con = getConnection(); // implicitly "begin"
>     try {
>        insertFrameworkLevelState(con);
>        insertApplicationLevelState(con);
>        con.commit();
>        publishNewState();
>     } catch (Throwable ex) {
>        con.rollback();
>     }
> }
>
> With the current implementation, it is possible, that the control flow
> reaches "publishNewState()" without the changes done in
> "insertFrameworkLevelState()" have been made persistent - without the
> framework-level code (which is everything except
> "insertApplicationLevelState()") being able to detect the problem (e.g.
> if "insertApplicationLevelState()" tries add a null into a non-null
> column catching the exception or any other application-level error that
> is not properly handled through safepoints).
>
>  From a framework's perspective, this behavior is absolutely
> unacceptable. Here, the framework-level code sees a database that
> commits successfully but does not make its changes persistent.
> Therefore, I don't think that altering this behavior has more downside
> than upside.

I am not sure that this example really proves anything. If
insertFrameworkLevelState(con), insertApplicationLevelState(con), and
con.commit() throw exceptions, or if they return a status code and you
check it and throw an exception if it's not what you expect, then it
will work. If database errors that occur during those operations are
ignored, then you've got a problem, but it does not seem necessary to
change the behavior of the database to fix that problem.

I think one of the larger issues in this area is that people have
script that go:

BEGIN;
-- do stuff
COMMIT;
BEGIN;
-- do more stuff
COMMIT;

...and they run these scripts by piping them into psql. Now, if the
COMMIT leaves the session in a transaction state, this is going to
have pretty random behavior. Like your example, this can be fixed by
having proper error checking in the application, but that does require
that your application is something more than a psql script.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Feb 14, 2020 at 1:04 PM Dave Cramer <davecramer@postgres.rocks> wrote:
> Thing is that con.commit() DOESN'T return a status code, nor does it throw an exception as we silently ROLLBACK
here.

Why not? There's nothing keeping the driver from doing either of those
things, is there? I mean, if using libpq, you can use PQcmdStatus() to
get the command tag, and find out whether it's COMMIT or ROLLBACK. If
you're implementing the wire protocol directly, you can do something
similar.

https://www.postgresql.org/docs/current/libpq-exec.html#LIBPQ-EXEC-NONSELECT

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Feb 14, 2020 at 2:08 PM Dave Cramer <davecramer@postgres.rocks> wrote:
> Well now you are asking the driver to re-interpret the results in a different way than the server which is not what
wetend to do.
 
>
> The server throws an error we throw an error. We really aren't in the business of re-interpreting the servers
responses.

I don't really see a reason why the driver has to throw an exception
if and only if there is an ERROR on the PostgreSQL side. But even if
you want to make that rule for some reason, it doesn't preclude
correct behavior here. All you really need is to have con.commit()
return some indication of what the command tag was, just as, say, psql
would do. If the server provides you with status information and you
throw it out instead of passing it along to the application, that's
not ideal.

Another thing that kinda puzzles me about this situation is that, as
far as I know, the only time COMMIT returns ROLLBACK is if the
transaction has already previously reported an ERROR. But if an ERROR
gets turned into an exception, then, in the code snippet previously
provided, we'd never reach con.commit() in the first place.

I'm not trying to deny that you might find some other server behavior
more convenient. You might. And, to Vik's original point, it might be
more compliant with the spec, too. But since changing that would have
a pretty big blast radius at this stage, I think it's worth trying to
make things work as well as they can with the server behavior that we
already have. And I don't really see anything preventing the driver
from doing that technically. I don't understand the idea that the
driver is somehow not allowed to notice the command tag.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Dave Cramer <davecramer@postgres.rocks> writes:
> On Fri, 14 Feb 2020 at 14:37, Robert Haas <robertmhaas@gmail.com> wrote:
>> I'm not trying to deny that you might find some other server behavior
>> more convenient. You might. And, to Vik's original point, it might be
>> more compliant with the spec, too. But since changing that would have
>> a pretty big blast radius at this stage, I think it's worth trying to
>> make things work as well as they can with the server behavior that we
>> already have. And I don't really see anything preventing the driver
>> from doing that technically. I don't understand the idea that the
>> driver is somehow not allowed to notice the command tag.

> We have the same blast radius.
> I have offered to make the behaviour requested dependent on a configuration
> parameter but apparently this is not sufficient.

Nope, that is absolutely not happening.  We learned very painfully, back
around 7.3 when we tried to put in autocommit on/off, that if server
behaviors like this are configurable then most client code has to be
prepared to work with every possible setting.  The argument that "you can
just set it to work the way you expect" is a dangerous falsehood.  I see
no reason to think that a change like this wouldn't suffer the same sort
of embarrassing and expensive failure that autocommit did.

            regards, tom lane

On 2020-Feb-14, Dave Cramer wrote:

> I offered to make the behaviour in the JDBC driver dependent on a
> configuration parameter

Do you mean "if con.commit() results in a rollback, then an exception is
thrown, unless the parameter XYZ is set to PQR"?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Am 14.02.2020 um 20:36 schrieb Robert Haas:
> On Fri, Feb 14, 2020 at 2:08 PM Dave Cramer <davecramer@postgres.rocks> wrote:
>> Well now you are asking the driver to re-interpret the results in a different way than the server which is not what
wetend to do.
 
>>
>> The server throws an error we throw an error. We really aren't in the business of re-interpreting the servers
responses.
> I don't really see a reason why the driver has to throw an exception
> if and only if there is an ERROR on the PostgreSQL side. But even if
> you want to make that rule for some reason, it doesn't preclude
> correct behavior here. All you really need is to have con.commit()
> return some indication of what the command tag was, just as, say, psql
> would do.

I think, this would be an appropriate solution. PostgreSQL reports the 
"unsuccessful" commit through the "ROLLBACK" status code and the driver 
translates this into a Java SQLException, because this is the only way 
to communicate the "non-successfullness" from the void commit() method. 
Since the commit() was not successful, from the API point of view this 
is an error and it is fine to report this using an exception.

Am 14.02.2020 um 21:07 schrieb Tom Lane:
> Dave Cramer <davecramer@postgres.rocks> writes:
>> We have the same blast radius.
>> I have offered to make the behaviour requested dependent on a configuration
>> parameter but apparently this is not sufficient.
> Nope, that is absolutely not happening.  We learned very painfully, back
> around 7.3 when we tried to put in autocommit on/off, that if server
> behaviors like this are configurable then most client code has to be
> prepared to work with every possible setting.  The argument that "you can
> just set it to work the way you expect" is a dangerous falsehood.  I see
> no reason to think that a change like this wouldn't suffer the same sort
> of embarrassing and expensive failure that autocommit did.

Doing this in a (non-default) driver setting is not ideal, because I 
expect do be notified *by default* from a database (driver) if a commit 
was not successful (and since the API is void, the only notification 
path is an exception). We already have a non-default option named 
"autosafe", which fixes the problem somehow.

If we really need both behaviors ("silently ignore failed commits" and 
"notify about failed commits") I would prefer adding a 
backwards-compatible option 
"silently-ignore-failed-commit-due-to-auto-rollback" (since it is a 
really aburd setting from my point of view, since consistency is at risk 
if this happens - the worst thing to expect from a database).

Regards,  Bernhard

On 2020-Feb-14, Dave Cramer wrote:

> On Fri, 14 Feb 2020 at 15:19, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
> 
> > Do you mean "if con.commit() results in a rollback, then an exception is
> > thrown, unless the parameter XYZ is set to PQR"?
> 
> No. JDBC has a number of connection parameters which change internal
> semantics of various things.
> 
> I was proposing to have a connection parameter which would be
> throwExceptionOnFailedCommit (or something) which would do what it says.
> 
> None of this would touch the server. It would just change the driver
> semantics.

That's exactly what I was saying.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, 17 Feb 2020 at 13:02, Haumacher, Bernhard <haui@haumacher.de> wrote:

... would be an appropriate solution. PostgreSQL reports the
"unsuccessful" commit through the "ROLLBACK" status code and the driver
translates this into a Java SQLException, because this is the only way
to communicate the "non-successfullness" from the void commit() method.
Since the commit() was not successful, from the API point of view this
is an error and it is fine to report this using an exception.

Well it doesn't always report the unsuccessful commit as a rollback sometimes it says

"there is no transaction" depending on what happened in the transaction

Also when there is an error there is also a status provided by the backend. 

Since this is not an error to the backend there is no status that the exception can provide.

Doing this in a (non-default) driver setting is not ideal, because I
expect do be notified *by default* from a database (driver) if a commit
was not successful (and since the API is void, the only notification
path is an exception). We already have a non-default option named
"autosafe", which fixes the problem somehow.

The challenge with making this the default, is as Tom noted, many other people don't expect this.

Nobody expects a database reporting a successful commit, while it internally rolled back.

If there is code out there depending on this bug, it is fair to provide a backwards-compatible option to re-activate this unexpected behavior.

What many other frameworks do is have vendor specific behaviour. 

Perhaps writing a proxying driver might solve the problem?

That's exactly what we do - extending our database abstraction layer to work around database-specific interpretations of the JDBC API.

But of cause, the abstraction layer is not able to reconstruct an error from a commit() call, that has been dropped by the driver. Of cause, I could try to insert another dummy entry into a dummy table immediately before each commit to get again the exception reporting that the transaction is in rollback-only-mode... but this does not sound reasonable to me.

If we really need both behaviors ("silently ignore failed commits" and
"notify about failed commits") I would prefer adding a
backwards-compatible option
"silently-ignore-failed-commit-due-to-auto-rollback" (since it is a
really aburd setting from my point of view, since consistency is at risk
if this happens - the worst thing to expect from a database).

The error has been reported to the client. At this point the client is expected to do a rollback.

As I explained, there is not "the client" but there are several software layers - and the error only has been reported to some of these layers that may decide not to communicate the problem down the road. Therefore, the final commit() must report the problem again.

Best regard, Bernhard

On Sun, Feb 23, 2020 at 11:11 AM Shay Rojansky <roji@roji.org> wrote:
> I'd like to second Dave on this, from the .NET perspective - actual client access is done via standard drivers in
almostall cases, and these drivers generally adhere to database API abstractions (JDBC for Java, ADO.NET for .NET, and
soon). AFAIK, in almost all such abstractions, commit can either complete (implying success) or throw an exception -
thereis no third way to return a status code. It's true that a driver may expose NOTICE/WARNING messages via some other
channel(Npgsql emits .NET events for these), but this is a separate message "channel" that is disconnected API-wise
fromthe commit; this makes the mechanism very "undiscoverable". 

I'm still befuddled here. First, to repeat what I said before, the
COMMIT only returns a ROLLBACK command tag if there's been a previous
ERROR. So, if you haven't ignored the prior ERROR, you should be fine.
Second, there's nothing to keep the driver itself from translating
ROLLBACK into an exception, if that's more convenient for some
particular driver. Let's go back to Bernhard's example upthred:

composeTransaction() {
    Connection con = getConnection(); // implicitly "begin"
    try {
       insertFrameworkLevelState(con);
       insertApplicationLevelState(con);
       con.commit();
       publishNewState();
    } catch (Throwable ex) {
       con.rollback();
    }
}

If insertFrameworkLevelState() or insertApplicationLevelState()
perform database operations that fail, then an exception should be
thrown and we should end up at con.rollback(), unless there is an
internal catch block inside those functions that swallows the
exception, or unless the JDBC driver ignores the error from the
server. If those things succeed, then COMMIT could still fail with an
ERROR but it shouldn't return ROLLBACK. But, for extra security,
con.commit() could be made to throw an exception if the command tag
returned by COMMIT is not COMMIT. It sounds like Dave doesn't want to
do that, but it would solve this problem without requiring a server
behavior change.

Actually, an even better idea might be to make the driver error out
when the transaction is known to be in a failed state when you enter
con.commit(). The server does return an indication after each command
as to whether the session is in a transaction and whether that
transaction is in a failed state. That's how the %x escape sequence
just added to the psql prompt works. So, suppose the JDBC driver
tracked that state like libpq does. insertFrameworkLevelState() or
insertApplicationLevelState() throws an exception, which is internally
swallowed. Then you reach con.commit(), and it says, nope, can't do
that, we're in a failed state, and so an exception is thrown. Then
when we reach con.rollback() we're still inside a transaction, it gets
rolled back, and everything works just as expected.

Or, alternatively, the JDBC driver could keep track of the fact that
it had thrown an exception ITSELF, without paying any attention to
what the server told it, and if it saw con.commit() after raising an
exception, it could raise another exception (or re-raise the same
one). That would also fix it.

> Asking drivers to do this at the client have the exact same breakage impact as the server change, since the
user-visiblebehavior changes in the same way (the change is just shifted from server to driver). What's worse is that
everydriver now has to reimplement the same new logic, and we'd most probably end up with some drivers doing it in some
languages,and others not doing it in others (so behavioral differences). 

Well, it seems quite possible that there are drivers and applications
that don't have this issue; I've never had a problem with this
behavior, and I've been using PostgreSQL for something like two
decades, and I believe that the sketch above could be used to get the
desired behavior in current releases of PostgreSQL with no server code
change. If we did change the server behavior, it seems unlikely that
every driver would adjust their behavior to the new server behavior
all at once and that they would all get it right while also all
preserving backward compatibility with current releases in case a
newer driver is used with an older server. I don't think that's
likely. What would probably happen is that many drivers would ignore
the change, leaving applications to cope with the differences between
server versions, and some would change the driver behavior
categorically, breaking compatibility with older server versions, and
some would make mistakes in implementing support for the new behavior.
And maybe we would also find that the new behavior isn't ideal for
everybody any more than the current behavior is ideal for everybody.

I am really struggling to see why this is anything but a bug in the
JDBC driver. The problem is that the application doesn't know that the
transaction has failed, but the server has returned not one, not two,
but three indications of failure. First, it returned an error, which I
guess the JDBC driver turns into an exception - but it does not,
before throwing that exception, remember that the current transaction
is failed. Second, it will thereafter report that the transaction is
in a failed state, both immediately after the error and upon every
subsequent operation that does not get the server out of the
transaction. It sounds like the JDBC driver ignores this information.
Third, the attempt at COMMIT will return a ROLLBACK command tag, which
Dave said that the driver does ignore. That's a lot of stuff that the
driver could do but isn't doing. So what this boils down to, from my
perspective, is not that the driver behavior in the face of errors
can't be made correct with the existing semantics, but that the driver
would find it more convenient if PostgreSQL reported those errors in a
somewhat different way. I think that's a fair criticism, but I don't
think it's a sufficient reason to change the behavior.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 24/02/2020 02:31, Robert Haas wrote:
> I am really struggling to see why this is anything but a bug in the
> JDBC driver.

I can follow your logic for it being a bug in the JDBC driver, but
"anything but"?  No, this is (also) an undocumented violation of SQL.
-- 
Vik Fearing

On Mon, Feb 24, 2020 at 7:29 AM Dave Cramer <davecramer@postgres.rocks> wrote:
> Well the driver really isn't in the business of changing the semantics of the server.

I mean, I just can't agree with that way of characterizing it. It
seems clear enough that the driver not only should not change the
semantics of the server, but that it cannot. It can, however, decide
which of the things that the server might do (or that the application
connected to it might do) ought to result in it throwing an exception.
And a slightly different set of decisions here would produce the
desired behavior instead of behavior which is not desired.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Feb 24, 2020 at 1:31 PM Vik Fearing <vik@postgresfriends.org> wrote:
> On 24/02/2020 02:31, Robert Haas wrote:
> > I am really struggling to see why this is anything but a bug in the
> > JDBC driver.
>
> I can follow your logic for it being a bug in the JDBC driver, but
> "anything but"?  No, this is (also) an undocumented violation of SQL.

Well, that's a fair point. I withdraw my previous statement. Instead,
I wish to argue that:

1. This problem can definitely be fixed in any given driver without
changing the behavior of the server.

2. It would be better to fix the driver than the server because this
behavior is very old and there are probably many applications (and
perhaps some drivers) that rely on it, and changing the server would
break them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Feb 24, 2020 at 1:56 PM Shay Rojansky <roji@roji.org> wrote:
> As Dave wrote, the problem here isn't with the driver, but with framework or user-code which swallows the initial
exceptionand allows code to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does surface PostgreSQL
errorsas exceptions, and internally tracks the transaction status provided in the CommandComplete message. That means
usershave the ability - but not the obligation - to know about failed transactions, and some frameworks or user coding
patternscould lead to a commit being done on a failed transaction. 

Agreed. All of that can be fixed in the driver, though.

> If we think the current *user-visible* behavior is problematic (commit on failed transaction completes without
throwing),then the only remaining question is where this behavior should be fixed - at the server or at the driver. As
Iwrote above, from the user's perspective it makes no difference - the change would be identical (and just as breaking)
eitherway. So while drivers *could* implement the new behavior, what advantages would that have over doing it at the
server?Some disadvantages do seem clear (repetition of the logic across each driver - leading to inconsistency across
drivers,changing semantics at the driver by turning a non-error into an exception...). 

The advantage is that it doesn't cause a compatibility break.

> > Well, it seems quite possible that there are drivers and applications that don't have this issue; I've never had a
problemwith this behavior, and I've been using PostgreSQL for something like two decades [...] 
>
> If we are assuming that most user code is already written to avoid committing on failed transactions (by tracking
transactionstate etc.), then making this change at the server wouldn't affect those applications; the only applications
affectedwould be those that do commit on failed transactions today, and it could be argued that those are likely to be
brokentoday (since drivers today don't really expose the rollback in an accessible/discoverable way). 

libpq exposes it just fine, so I think you're overgeneralizing here.

As I said upthread, I think one of the things that would be pretty
badly broken by this is psql -f something.sql, where something.sql
contains a series of blocks of the form "begin; something; something;
something; commit;". Right now whichever transactions succeed get
committed. With the proposed change, if one transaction block fails,
it'll merge with all of the following blocks. You may think that
nobody is doing this sort of thing, but I think people are, and that
they will come after us with pitchforks if we break it.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Feb 24, 2020 at 06:04:28PM +0530, Robert Haas wrote:
> On Mon, Feb 24, 2020 at 1:56 PM Shay Rojansky <roji@roji.org> wrote:
> > As Dave wrote, the problem here isn't with the driver, but with framework or user-code which swallows the initial
exceptionand allows code to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does surface PostgreSQL
errorsas exceptions, and internally tracks the transaction status provided in the CommandComplete message. That means
usershave the ability - but not the obligation - to know about failed transactions, and some frameworks or user coding
patternscould lead to a commit being done on a failed transaction.
 
> 
> Agreed. All of that can be fixed in the driver, though.
> 
> > If we think the current *user-visible* behavior is problematic (commit on failed transaction completes without
throwing),then the only remaining question is where this behavior should be fixed - at the server or at the driver. As
Iwrote above, from the user's perspective it makes no difference - the change would be identical (and just as breaking)
eitherway. So while drivers *could* implement the new behavior, what advantages would that have over doing it at the
server?Some disadvantages do seem clear (repetition of the logic across each driver - leading to inconsistency across
drivers,changing semantics at the driver by turning a non-error into an exception...).
 
> 
> The advantage is that it doesn't cause a compatibility break.
> 
> > > Well, it seems quite possible that there are drivers and applications that don't have this issue; I've never had
aproblem with this behavior, and I've been using PostgreSQL for something like two decades [...]
 
> >
> > If we are assuming that most user code is already written to avoid committing on failed transactions (by tracking
transactionstate etc.), then making this change at the server wouldn't affect those applications; the only applications
affectedwould be those that do commit on failed transactions today, and it could be argued that those are likely to be
brokentoday (since drivers today don't really expose the rollback in an accessible/discoverable way).
 
> 
> libpq exposes it just fine, so I think you're overgeneralizing here.
> 
> As I said upthread, I think one of the things that would be pretty
> badly broken by this is psql -f something.sql, where something.sql
> contains a series of blocks of the form "begin; something; something;
> something; commit;". Right now whichever transactions succeed get
> committed. With the proposed change, if one transaction block fails,
> it'll merge with all of the following blocks. You may think that
> nobody is doing this sort of thing, but I think people are, and that
> they will come after us with pitchforks if we break it.

I'm doing it, and I don't know about pitchforks, but I do know about
suddenly needing to rewrite (and re-test, and re-integrate, and
re-test some more) load-bearing code, and I'm not a fan of it.

If we'd done this from a clean sheet of paper, it would have been the
right decision. We're not there, and haven't been for decades.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 24/02/2020 18:37, David Fetter wrote:

> If we'd done this from a clean sheet of paper, it would have been the
> right decision. We're not there, and haven't been for decades.

OTOH, it's never too late to do the right thing.
-- 
Vik Fearing

On Mon, Feb 24, 2020 at 06:40:16PM +0100, Vik Fearing wrote:
> On 24/02/2020 18:37, David Fetter wrote:
> 
> > If we'd done this from a clean sheet of paper, it would have been the
> > right decision. We're not there, and haven't been for decades.
> 
> OTOH, it's never too late to do the right thing.

Some right things take a lot of prep work in order to actually be
right things. This is one of them.  Defaulting to SERIALIZABLE
isolation is another.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Sun, Feb 23, 2020 at 7:59 PM Dave Cramer <davecramer@postgres.rocks> wrote:
>
> I think the fact that this is a violation of the SQL SPEC lends considerable credence to the argument for changing
thebehaviour.
 
> Since this can lead to losing a transaction I think there is even more reason to look at changing the behaviour.

The assumption that COMMIT terminates the transaction is going to be
deeply embedded into many applications.  It's just too convenient not
to rely on.  For example, I maintain a bash based deployment framework
that assembles large SQL files from bit and pieces and tacks a COMMIT
at the end.  It's not *that* much work to test for failure and add a
rollback but it's the kind of surprise our users hate during the
upgrade process.

Over the years we've tightened the behavior of postgres to be inline
with the spec (example: Tom cleaned up the row-wise comparison
behavior in 8.2) but in other cases we had to punt (IS NULL/coalesce
disagreement over composites for example), identifier case sensitivity
etc.  The point is, changing this stuff can be really painful and we
have to evaluate the benefits vs the risks.

My biggest sense of alarm with the proposed change is that it could
leave applications in a state where the transaction is hanging there
it could previously assume it had resolved; this could be catastrophic
in impact in certain real world scenarios.  Tom is right, a GUC is the
equivalent of "sweeping the problem under the wrong" (if you want
examples of the long term consequences of that vision read through
this: https://dev.mysql.com/doc/refman/8.0/en/timestamp-initialization.html).
  The value proposition of the change is however a little light
relative to the risks IMO.

I do think we need to have good page summarizing non-spec behaviors in
the documentation however.

merlin

On 12/02/2020 00:27, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> On 11/02/2020 23:35, Tom Lane wrote:
>>> So I assume you're imagining that that would leave us still in
>>> transaction-aborted state, and the session is basically dead in
>>> the water until the user thinks to issue ROLLBACK instead?
> 
>> Actually, I was imagining that it would end the transaction as it does
>> today, just with an error code.
>> This is backed up by General Rule 9 which says "The current
>> SQL-transaction is terminated."
> 
> Hm ... that would be sensible, but I'm not entirely convinced.  There
> are several preceding rules that say that an exception condition is
> raised, and normally you can stop reading at that point; nothing else
> is going to happen.  If COMMIT acts specially in this respect, they
> ought to say so.

Reading some more, I believe they do say so.

SQL:2016-2 Section 4.41 SQL-transactions:

    If an SQL-transaction is terminated by a <rollback statement> or
    unsuccessful execution of a <commit statement>, then all changes
    made to SQL-data or schemas by that SQL-transaction are canceled.

This to me says that an unsuccessful COMMIT still terminates the
transaction.
-- 
Vik Fearing

On Mon, Feb 24, 2020 at 4:06 PM Vladimir Sitnikov
<sitnikov.vladimir@gmail.com> wrote:
>
> Merlin>My biggest sense of alarm with the proposed change is that it could
> Merlin>leave applications in a state where the transaction is hanging there
>
> How come?
> The spec says commit ends the transaction.
> Can you please clarify where the proposed change leaves a hanging transaction?
>
> Just in case, the proposed change is as follows:
>
> postgres=# begin;
> BEGIN
> postgres=# aslkdfasdf;
> ERROR:  syntax error at or near "aslkdfasdf"
> LINE 1: aslkdfasdf;
>         ^
> postgres=# commit;
> ROLLBACK   <-- this should be replaced with "ERROR: can't commit the transaction because ..."
> postgres=# commit;
> WARNING:  there is no transaction in progress  <-- this should be as it is currently. Even if commit throws an error,
thetransaction should be terminated.
 
> COMMIT

Ok, you're right; I missed the point in that it's not nearly as bad as
I thought you were suggesting (to treat commit as bad statement) but
the transaction would still terminate.   Still, this is very sensitive
stuff, do you think most common connection poolers would continue to
work after making this change?

merlin

On Mon, Feb 24, 2020 at 6:40 PM Dave Cramer <davecramer@postgres.rocks> wrote:
> Fair enough. What I meant to say was that the driver isn't in the business of providing different semantics than the
serverprovides.
 

Still don't agree. The server doesn't make any decision about what
semantics the driver has to provide. The driver can do whatever it
wants. If what it does makes users sad, then maybe it ought to do
something different.

Now, of course, it's also true that if what the server does makes
users sad, maybe the server should do something different. But I think
you're vastly underestimating the likely impact on other users and
drivers of making this change. That is a guess, and like any guess,
may be wrong. But it is still what I think.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, Feb 25, 2020 at 12:47 PM Vladimir Sitnikov
<sitnikov.vladimir@gmail.com> wrote:
> Noone suggested that "commit leaves the session in a transaction state".
> Of course, every commit should terminate the transaction.
> However, if a commit fails (for any reason), it should produce the relevant ERROR that explains what went wrong
ratherthan silently doing a rollback.
 

OK, I guess I misinterpreted the proposal. That would be much less
problematic -- any driver or application that can't handle ERROR in
response to an attempted COMMIT would be broken already.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Tue, 2020-02-25 at 13:25 +0530, Robert Haas wrote:
> On Tue, Feb 25, 2020 at 12:47 PM Vladimir Sitnikov
> <sitnikov.vladimir@gmail.com> wrote:
> > Noone suggested that "commit leaves the session in a transaction state".
> > Of course, every commit should terminate the transaction.
> > However, if a commit fails (for any reason), it should produce the relevant ERROR that explains what went wrong
ratherthan silently doing a rollback.
 
> 
> OK, I guess I misinterpreted the proposal. That would be much less
> problematic -- any driver or application that can't handle ERROR in
> response to an attempted COMMIT would be broken already.

I agree with that.

There is always some chance that someone relies on COMMIT not
throwing an error when it rolls back, but I think that throwing an
error is actually less astonishing than *not* throwing one.

So, +1 for the proposal from me.

Yours,
Laurenz Albe

Am 24.02.2020 um 13:34 schrieb Robert Haas:
> As I said upthread, I think one of the things that would be pretty
> badly broken by this is psql -f something.sql, where something.sql
> contains a series of blocks of the form "begin; something; something;
> something; commit;". Right now whichever transactions succeed get
> committed. With the proposed change, if one transaction block fails,
> it'll merge with all of the following blocks.


No, that's *not* true.

The only difference with the proposed change would be another error in 
the logs for the commit following the block with the failed insert. 
Note: Nobody has suggested that the commit that returns with an error 
should not end the transaction. Do just the same as with any other 
commit error in response to a constraint violation!


Am 24.02.2020 um 18:53 schrieb David Fetter:
> On Mon, Feb 24, 2020 at 06:40:16PM +0100, Vik Fearing wrote:
>> On 24/02/2020 18:37, David Fetter wrote:
>>> If we'd done this from a clean sheet of paper, it would have been the
>>> right decision. We're not there, and haven't been for decades.
>> OTOH, it's never too late to do the right thing.
> Some right things take a lot of prep work in order to actually be
> right things. This is one of them.  Defaulting to SERIALIZABLE
> isolation is another.


Here the proposed changes is really much much less noticable - please 
report the error (again) instead of giving an incomprehensible status 
code. Nothing else must be changed - the failing commit should do the 
rollback and end the transaction - but it should report this situation 
as an error!

Regards Bernhard

On 25/02/2020 12:11, Laurenz Albe wrote:
> On Tue, 2020-02-25 at 13:25 +0530, Robert Haas wrote:
>> On Tue, Feb 25, 2020 at 12:47 PM Vladimir Sitnikov
>> <sitnikov.vladimir@gmail.com> wrote:
>>> Noone suggested that "commit leaves the session in a transaction state".
>>> Of course, every commit should terminate the transaction.
>>> However, if a commit fails (for any reason), it should produce the relevant ERROR that explains what went wrong
ratherthan silently doing a rollback.
 
>>
>> OK, I guess I misinterpreted the proposal. That would be much less
>> problematic -- any driver or application that can't handle ERROR in
>> response to an attempted COMMIT would be broken already.
> 
> I agree with that.
> 
> There is always some chance that someone relies on COMMIT not
> throwing an error when it rolls back, but I think that throwing an
> error is actually less astonishing than *not* throwing one.
> 
> So, +1 for the proposal from me.

I started this thread for some discussion and hopefully a documentation
patch.  But now I have moved firmly into the +1 camp.  COMMIT should
error if it can't commit, and then terminate the (aborted) transaction.
-- 
Vik Fearing

Dave Cramer <davecramer@postgres.rocks> writes:
> OK, here is a patch that actually doesn't leave the transaction in a failed
> state but emits the error and rolls back the transaction.

> This is far from complete as it fails a number of tests  and does not cover
> all of the possible paths.
> But I'd like to know if this is strategy will be acceptable ?

I really don't think that changing the server's behavior here is going to
fly.  The people who are unhappy that we changed it are going to vastly
outnumber the people who are happy.  Even the people who are happy are not
going to find that their lives are improved all that much, because they'll
still have to deal with old servers with the old behavior for the
foreseeable future.

Even granting that a behavioral incompatibility is acceptable, I'm not
sure how a client is supposed to be sure that this "error" means that a
rollback happened, as opposed to real errors that prevented any state
change from occurring.  (A trivial example of that is misspelling the
COMMIT command; which I'll grant is unlikely in practice.  But there are
less-trivial examples involving internal server malfunctions.)  The only
way to be sure you're out of the transaction is to check the transaction
state that's sent along with ReadyForQuery ... but if you need to do
that, it's not clear why we should change the server behavior at all.

I also don't think that this scales to the case of subtransaction
commit/rollback.  That should surely act the same, but your patch doesn't
change it.

Lastly, introducing a new client-visible message level seems right out.
That's a very fundamental protocol break, independently of all else.
And if it's "not really an error", then how is this any more standards
compliant than before?

            regards, tom lane

On 26/02/2020 22:22, Tom Lane wrote:
> Dave Cramer <davecramer@postgres.rocks> writes:
>> OK, here is a patch that actually doesn't leave the transaction in a failed
>> state but emits the error and rolls back the transaction.
> 
>> This is far from complete as it fails a number of tests  and does not cover
>> all of the possible paths.
>> But I'd like to know if this is strategy will be acceptable ?
> 
> I really don't think that changing the server's behavior here is going to
> fly.  The people who are unhappy that we changed it are going to vastly
> outnumber the people who are happy.  Even the people who are happy are not
> going to find that their lives are improved all that much, because they'll
> still have to deal with old servers with the old behavior for the
> foreseeable future.

Dealing with old servers for a while is something that everyone is used to.

> Even granting that a behavioral incompatibility is acceptable, I'm not
> sure how a client is supposed to be sure that this "error" means that a
> rollback happened, as opposed to real errors that prevented any state
> change from occurring.

Because the error is a Class 40 — Transaction Rollback.

My original example was:

postgres=!# commit;
ERROR:  40P00: transaction cannot be committed
DETAIL:  First error was "42601: syntax error at or near "error"".


> (A trivial example of that is misspelling the
> COMMIT command; which I'll grant is unlikely in practice.  But there are
> less-trivial examples involving internal server malfunctions.)

Misspelling the COMMIT command is likely a syntax error, which is Class
42.  Can you give one of those less-trivial examples please?

> The only
> way to be sure you're out of the transaction is to check the transaction
> state that's sent along with ReadyForQuery ... but if you need to do
> that, it's not clear why we should change the server behavior at all.

How does this differ from the deferred constraint violation example you
provided early on in the thread?  That gave the error 23505 and
terminated the transaction.  If you run the same scenario with the
primary key immediate, you get the *exact same error* but the
transaction is *not* terminated!

I won't go so far as to suggest we change all COMMIT errors to Class 40
(as the spec says), but I'm thinking it very loudly.

> I also don't think that this scales to the case of subtransaction
> commit/rollback.  That should surely act the same, but your patch doesn't
> change it.

How does one commit a subtransaction?

> Lastly, introducing a new client-visible message level seems right out.
> That's a very fundamental protocol break, independently of all else.

Yeah, this seemed like a bad idea to me, too.
-- 
Vik Fearing

On Wed, Feb 26, 2020 at 11:53 PM Vladimir Sitnikov
<sitnikov.vladimir@gmail.com> wrote:
> Pushing full SQL parser to the driver is not the best idea taking into the account the extensibility the core has.

That wouldn't be necessary. You could just do strcmp() on the command tag.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar 6, 2020 at 11:55 AM Dave Cramer <davecramer@postgres.rocks> wrote:
> There have been some arguments that the client can fix this easily.
>
> Turns out it is not as easy as one might think.
>
> If the client (in this case JDBC) uses conn.commit() then yes relatively easy as we know that commit is being
executed.

Right...

> however if the client executes commit using direct SQL and possibly multiplexes a number of commands we would have to
parsethe SQL to figure out what is being sent. This could include a column named commit_date or a comment with commit
embeddedin it. It really doesn't make sense to have a full fledged PostgreSQL SQL parser in every client. This is
somethingthe server does very well. 

That's true. If the command tag is either COMMIT or ROLLBACK then the
statement was either COMMIT or ROLLBACK, but Vladimir's example query
/*commit*/rollback does seem like a pretty annoying case. I was
assuming that the JDBC driver required use of con.commit() in the
cases we care about, but perhaps that's not so.

> There has been another argument that we can simply check the transaction state after we get the ReadyForQuery
response,however this is set to IDLE after the subsequent ROLLBACK  so that doesn't work either. 

I assumed you'd look at the *previous* ReadyForQuery message and see
whether it said "in transaction" ('T') or "failed in transaction"
('E'). If the transaction was failed, then only rollback is possible,
but if it's not, then either commit or rollback is possible.

But I agree that if you don't know what you command you sent, and have
to deal with users who send things like /*commit*/rollback, then the
current reporting is not good enough. If the command tag for a commit
that got converted into a rollback were distinct from the command tag
that you get from a deliberate rollback, then it would be file; say if
we sent ROLLBACK COMMIT for one and just ROLLBACK for the other, for
example. But that's not how it works.

I think you can still fix the con.commit() case. But users issuing
ad-hoc SQL that may contain comments intended to snipe the driver
seems like it does require a server-side change.

> Additionally in section 52.2.2 of the docs it states:
>
> A frontend must be prepared to accept ErrorResponse and NoticeResponse messages whenever it is expecting any other
typeof message. See also Section 52.2.6 concerning messages that the backend might generate due to outside events. 
>
> Recommended practice is to code frontends in a state-machine style that will accept any message type at any time that
itcould make sense, rather than wiring in assumptions about the exact sequence of messages. 
>
> Seems to me that this behaviour is already documented?

I don't understand what you're going for here.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Mar  6, 2020 at 01:12:10PM -0500, Robert Haas wrote:
> On Fri, Mar 6, 2020 at 11:55 AM Dave Cramer <davecramer@postgres.rocks> wrote:
> > There have been some arguments that the client can fix this easily.
> >
> > Turns out it is not as easy as one might think.
> >
> > If the client (in this case JDBC) uses conn.commit() then yes relatively easy as we know that commit is being
executed.
> 
> Right...
> 
> > however if the client executes commit using direct SQL and possibly multiplexes a number of commands we would have
toparse the SQL to figure out what is being sent. This could include a column named commit_date or a comment with
commitembedded in it. It really doesn't make sense to have a full fledged PostgreSQL SQL parser in every client. This
issomething the server does very well.
 
> 
> That's true. If the command tag is either COMMIT or ROLLBACK then the
> statement was either COMMIT or ROLLBACK, but Vladimir's example query
> /*commit*/rollback does seem like a pretty annoying case. I was
> assuming that the JDBC driver required use of con.commit() in the
> cases we care about, but perhaps that's not so.

Let me try to summarize where I think we are on this topic.

First, Vik reported that we don't follow the SQL spec when issuing a
COMMIT WORK in a failed transaction.  We return success and issue the
ROLLBACK command tag, rather than erroring.  In general, if we don't
follow the spec, we should either have a good reason, or the breakage to
match the spec is too severe.  (I am confused why this has not been
reported before.)

Second, someone suggested that if COMMIT throws an error, that future
statements would be considered to be in the same transaction block until
ROLLBACK is issued.  It was determined that this is not required, and
that the API should have COMMIT WORK on a failed transaction still exit
the transaction block.  This behavior is much more friendly for SQL
scripts piped into psql.

Third, the idea that individual interfaces, e.g. JDBC, should throw an
error in this case while the server just changes the COMMIT return tag
to ROLLBACK is confusing.  People regularly test SQL commands in the
server before writing applications or while debugging, and a behavior
mismatch would cause confusion.

Fourth, it is not clear how many applications would break if COMMIT
started issuing an error rather than return success a with ROLLBACK tag.
Certainly SQL scripts would be fine.  They would have one additional
error in the script output, but if they had ON_ERROR_STOP enabled, they
would have existed before the commit.  Applications that track statement
errors and issue rollbacks will be fine.  So, we are left with
applications that issue COMMIT and expect success after a transaction
block has failed.  Do we know how other database systems handle this?

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On Tue, Mar 17, 2020 at 07:15:05PM -0400, Dave Cramer wrote:
> On Tue, 17 Mar 2020 at 16:47, Bruce Momjian <bruce@momjian.us> wrote:
>     Third, the idea that individual interfaces, e.g. JDBC, should throw an
>     error in this case while the server just changes the COMMIT return tag
>     to ROLLBACK is confusing.  People regularly test SQL commands in the
>     server before writing applications or while debugging, and a behavior
>     mismatch would cause confusion.
> 
> 
> I'm not sure what you mean by this. The server would throw an error. 

I am saying it is not wise to have interfaces behaving differently than
the server, for the reasons stated above.

>     Fourth, it is not clear how many applications would break if COMMIT
>     started issuing an error rather than return success a with ROLLBACK tag.
>     Certainly SQL scripts would be fine.  They would have one additional
>     error in the script output, but if they had ON_ERROR_STOP enabled, they
>     would have existed before the commit.  Applications that track statement
>     errors and issue rollbacks will be fine.  So, we are left with
>     applications that issue COMMIT and expect success after a transaction
>     block has failed.  Do we know how other database systems handle this?
> 
> Well I know pgjdbc handles my patch fine without any changes to the code
> As I mentioned upthread 2 of the 3 go drivers already error if rollback is
> returned. 1 of them does not.

Good point.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EnterpriseDB                             https://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 3/30/20 6:05 PM, Dave Cramer wrote:
> So it appears this is currently languishing as unresolved and feature
> freeze is imminent.
> 
> What has to be done to get a decision one way or another before feature
> freeze.
> 
> I have provided a patch that could be reviewed and at least be considered
> in the commitfest.
> 
> Perhaps someone can review the patch and I can do whatever it takes to get
> it presentable ?


I don't know enough about that part of the code to give a meaningful
review, but I will give my full support to the patch.  (I hadn't
expressed an opinion either way yet.)
-- 
Vik Fearing

On Thu, 16 Apr 2020 at 21:16, Shay Rojansky <roji@roji.org> wrote:
> Npgsql would be fine. In fact, Npgsql doesn't have any specific expectations nor any specific logic around commit; it
assumeserrors may be returned for any command (COMMIT or otherwise), and surfaces those errors as .NET exceptions.
 

Hi all, I work on the pg8000 Python driver for Postgres and having
read through the thread I'd like to echo Shay Rojansky's comment and
say that pg8000 would be able to handle the behaviour resulting from
the proposed patch and I support the change of a call to commit()
*always* producing an error if it has failed. I can understand
people's reluctance in general to change server behaviour, but in this
case I think the good outweighs the bad. I think most people expected
the server to be behaving like this anyway.

Regards,

Tony.

On 8/4/20 12:19 PM, Dave Cramer wrote:
> Attached is the rebased patch for consideration.
>
>


It's a bit sad this has been hanging around so long without attention.


The previous discussion seems to give the patch a clean bill of health
for most/all of the native drivers. Are there any implications for libpq
based drivers such as DBD::Pg and psycopg2? How about for ecpg?


cheers


andrew


-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi,

thank you for your contribution.

I did notice that the cfbot [1] is failing for this patch.
Please try to address the issue for the upcoming commitfest.

Cheers,
//Georgios

[1] http://cfbot.cputube.org/dave-cramer.html

Hi,

I noticed that this patch fails on the cfbot.
For this, I changed the status to: 'Waiting on Author'.

Cheers,
//Georgios

The new status of this patch is: Waiting on Author

Hi,

this patch fails on the cfbot yet it has received an update during the current CF.

I will move it to the next CF and mark it there as Waiting on Author.

Cheers,
Georgios

The new status of this patch is: Needs review

Hi Dave,

On Tue, Dec 1, 2020 at 6:49 PM Georgios Kokolatos
<gkokolatos@protonmail.com> wrote:
>
> Hi,
>
> this patch fails on the cfbot yet it has received an update during the current CF.
>
> I will move it to the next CF and mark it there as Waiting on Author.
>

This patch has not been updated for almost 2 months. According to
cfbot test, the patch conflicts on only src/include/utils/elog.h.
Could you submit the rebased patch?

Regards,

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/

On Thu, Jan 7, 2021 at 11:29 PM Dave Cramer <davecramer@postgres.rocks> wrote:
>
> I could if someone wants to commit to reviewing it.
> I've updated it a number of times but it seems nobody wants to review it.

Since this has a long thread, how about summarizing what consensus we
reached and what discussion we still need if any so that new reviewers
can easily catch up? I think people who want to start reviewing are
likely to search the patch marked as "Needs Review". So I think
continuous updating and rebasing the patch would help get the patch
reviewed also in terms of that.

Regards,

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

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.

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

Yours,
Laurenz Albe

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,

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

On Tue, 2021-01-26 at 11:09 -0500, Dave Cramer wrote:
> On Tue, 26 Jan 2021 at 05:05, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
>
> > 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

Oh, I see.

After thinking some more about it, I think that COMMIT AND CHAIN would have
to change behavior: if COMMIT throws an error (because the transaction was
aborted), no new transaction should be started.  Everything else seems fishy:
the statement fails, but still starts a new transaction?

I guess that's also at fault for the unexpected result status that
Masahiko complained about in the other message.

So I think we should not introduce USER_ERROR at all.  It is too much
of a kluge: fail, but not really...

I guess that is one example for the incompatibilities that Tom worried
about upthread.  I am beginning to see his point better now.

Yours,
Laurenz Albe

On 1/26/21 6:20 PM, Laurenz Albe wrote:
> After thinking some more about it, I think that COMMIT AND CHAIN would have
> to change behavior: if COMMIT throws an error (because the transaction was
> aborted), no new transaction should be started.  Everything else seems fishy:
> the statement fails, but still starts a new transaction?

The standard is not clear (to me) on what exactly should happen here.
It says that if a <commit statement> is not successful then a <rollback
statement> is implied, but I don't see it say anything about whether the
AND CHAIN should be propagated too.

My vote is that COMMIT AND CHAIN should become ROLLBACK AND NO CHAIN.
-- 
Vik Fearing

On 1/26/21 6:34 PM, Vik Fearing wrote:
> On 1/26/21 6:20 PM, Laurenz Albe wrote:
>> After thinking some more about it, I think that COMMIT AND CHAIN would have
>> to change behavior: if COMMIT throws an error (because the transaction was
>> aborted), no new transaction should be started.  Everything else seems fishy:
>> the statement fails, but still starts a new transaction?
> 
> The standard is not clear (to me) on what exactly should happen here.
> It says that if a <commit statement> is not successful then a <rollback
> statement> is implied, but I don't see it say anything about whether the
> AND CHAIN should be propagated too.
> 
> My vote is that COMMIT AND CHAIN should become ROLLBACK AND NO CHAIN.

Hmm.  On the other hand, that means if the client isn't paying
attention, it'll start executing commands outside of a transaction which
will autocommit.  It might be better for a new transaction to be chained
and hopefully also fail because previous bits are missing.

I will hastily change my vote to "unsure".
-- 
Vik Fearing

On Tue, 2021-01-26 at 12:25 -0500, Dave Cramer wrote:
> > After thinking some more about it, I think that COMMIT AND CHAIN would have
> > to change behavior: if COMMIT throws an error (because the transaction was
> > aborted), no new transaction should be started.  Everything else seems fishy:
> > the statement fails, but still starts a new transaction?
> > 
> > I guess that's also at fault for the unexpected result status that
> > Masahiko complained about in the other message.
> 
>  
> I haven't had a look at the result status in libpq. For JDBC we don't see that. 
> We throw an exception when we get this error report. This is very consistent as the commit fails and we throw an
exception
> 
> > So I think we should not introduce USER_ERROR at all.  It is too much
> > of a kluge: fail, but not really...
> 
> What we do now is actually worse as we do not get an error report and we silently change commit to rollback.
> How is this better ?

I see your point from the view of the JDBC driver.

It just feels hacky - somewhat similar to what you say
above: don't go through the normal transaction rollback steps,
but issue an error message.

At least we should fake it well...

Yours,
Laurenz Albe

On 1/26/21 1:02 PM, Dave Cramer wrote:
> On Tue, 26 Jan 2021 at 12:46, Laurenz Albe <laurenz.albe@cybertec.at 
> <mailto:laurenz.albe@cybertec.at>> wrote:
> 
>     I see your point from the view of the JDBC driver.
> 
>     It just feels hacky - somewhat similar to what you say
>     above: don't go through the normal transaction rollback steps,
>     but issue an error message.
> 
>     At least we should fake it well...
> 
> OK, let me look into how we deal with COMMIT and CHAIN.
> 
> I can see some real issues with this as Vik pointed out.

Test are failing on the cfbot for this patch and it looks like a new 
patch is needed from Dave, at the least, so marking Waiting on Author.

Should we be considering this patch Returned with Feedback instead?

Regards,
-- 
-David
david@pgmasters.net

On 3/25/21 3:07 PM, Dave Cramer wrote:
> On Thu, 25 Mar 2021 at 12:04, David Steele <david@pgmasters.net 
> <mailto:david@pgmasters.net>> wrote:
> 
>     Test are failing on the cfbot for this patch and it looks like a new
>     patch is needed from Dave, at the least, so marking Waiting on Author.
> 
>     Should we be considering this patch Returned with Feedback instead?
> 
> Not sure, at this point the impetus for this is not getting a lot of 
> traction.
> Honestly I think the approach I took is too simple and I don't have the 
> inclination at the moment to
> rewrite it.

In that case Returned with Feedback is appropriate so I have done that.

Of course, the conversation can continue on this thread or a new one and 
when you have a new patch you can create a new CF entry.

Regards,
-- 
-David
david@pgmasters.net