Thread

1. Error on failed COMMIT

   Vik Fearing <vik@postgresfriends.org> — 2020-02-11T21:44:50Z

   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
   
   
   
   

2. Re: Error on failed COMMIT

   Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-11T22:35:13Z

   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
   
   
   
   

3. Re: Error on failed COMMIT

   Vik Fearing <vik@postgresfriends.org> — 2020-02-11T23:19:24Z

   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
   
   
   
   

4. Re: Error on failed COMMIT

   Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-11T23:27:44Z

   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
   
   
   
   

5. Re: Error on failed COMMIT

   Dave Cramer <davecramer@gmail.com> — 2020-02-12T00:23:18Z

   On Tue, 11 Feb 2020 at 17:35, Tom Lane <tgl@sss.pgh.pa.us> 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?
   >
   > > 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
   >
   
   interesting as if you do a commit after violating a not null it simply does
   a rollback
   with no warning whatsoever
   
   begin;
   BEGIN
   test=# insert into hasnull(i) values (null);
   ERROR:  null value in column "i" violates not-null constraint
   DETAIL:  Failing row contains (null).
   test=# commit;
   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?)
   >
   > Actually the bug reporter was looking for an upgrade from a warning to an
   ERROR
   
   I realize we are unlikely to change the behaviour however it would be
   useful if we
   did the same thing for all cases, and document this behaviour. We actually
   have places where
   we document where we don't adhere to the spec.
   
   Dave
   

6. Re: Error on failed COMMIT

   Haumacher, Bernhard <haui@haumacher.de> — 2020-02-13T07:38:18Z

   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
   
   
   
   
   

7. Re: Error on failed COMMIT

   Robert Haas <robertmhaas@gmail.com> — 2020-02-14T17:36:41Z

   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
   
   
   
   

8. Re: Error on failed COMMIT

   Dave Cramer <davecramer@postgres.rocks> — 2020-02-14T18:04:07Z

   On Fri, 14 Feb 2020 at 12:37, Robert Haas <robertmhaas@gmail.com> wrote:
   
   > 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.
   
   
   Thing is that con.commit() DOESN'T return a status code, nor does it throw
   an exception as we silently ROLLBACK here.
   
   As noted up thread it's somewhat worse as depending on how the transaction
   failed we seem to do different things
   
   In Tom's example we do issue a warning and say there is no transaction
   running. I would guess we silently rolled back earlier.
   In my example we don't issue the warning we just roll back.
   
   I do agree with Tom that changing this behaviour at this point would make
   things worse for more people than it would help so I am not advocating
   throwing an error here.
   
   I would however advocate for consistently doing the same thing with failed
   transactions
   
   Dave Cramer
   
   www.postgres.rocks
   

9. Re: Error on failed COMMIT

   Robert Haas <robertmhaas@gmail.com> — 2020-02-14T18:29:19Z

   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
   
   
   
   

10. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-14T19:08:20Z

    On Fri, 14 Feb 2020 at 13:29, Robert Haas <robertmhaas@gmail.com> wrote:
    
    > 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
    
    
    Well now you are asking the driver to re-interpret the results in a
    different way than the server which is not what we tend to do.
    
    The server throws an error we throw an error. We really aren't in the
    business of re-interpreting the servers responses.
    
    Dave Cramer
    www.postgres.rocks
    

11. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-14T19:36:53Z

    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 we tend 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
    
    
    
    

12. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-14T19:47:50Z

    On Fri, 14 Feb 2020 at 14:37, Robert Haas <robertmhaas@gmail.com> wrote:
    
    > 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 we tend 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.
    >
    
    Well con.commit() returns void :(
    
    
    >
    > 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.
    >
    
     The OP is building a framework where it's possible for the exception to be
    swallowed by the consumer of the framework.
    
    
    > 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.
    
    
    
    Dave Cramer
    www.postgres.rocks
    

13. Re: Error on failed COMMIT

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-14T20:07:33Z

    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
    
    
    
    

14. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-14T20:11:42Z

    On Fri, 14 Feb 2020 at 15:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
    
    > 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.
    
    
    I should have been more specific.
    
    I offered to make the behaviour in the JDBC driver dependent on a
    configuration parameter
    
    Dave Cramer
    www.postgres.rocks
    
    >
    >
    

15. Re: Error on failed COMMIT

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-14T20:19:50Z

    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
    
    
    
    

16. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-14T20:43:24Z

    On Fri, 14 Feb 2020 at 15:19, Alvaro Herrera <alvherre@2ndquadrant.com>
    wrote:
    
    > 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"?
    >
    
    
    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.
    
    
    Dave Cramer
    www.postgres.rocks
    

17. Re: Error on failed COMMIT

    Haumacher, Bernhard <haui@haumacher.de> — 2020-02-17T18:01:38Z

    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 we tend 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
    
    
    
    
    

18. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-17T22:12:04Z

    On Mon, 17 Feb 2020 at 13:02, Haumacher, Bernhard <haui@haumacher.de> wrote:
    
    > 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 we tend 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.
    >
    
    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.
    
    >
    > 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.
    >
    
    The challenge with making this the default, is as Tom noted, many other
    people don't expect this.
    
    I think the notion that every JDBC driver works exactly the same way for
    every API call is a challenge.
    Take for instance SERIALIZABLE transaction isolation.
    Only PostgreSQL actually implements it correctly. AFAIK Oracle SERIALIZABLE
    is actually REPEATABLE READ
    
    What many other frameworks do is have vendor specific behaviour.
    Perhaps writing a proxying driver might solve the problem?
    
    
    > 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.
    
    Regards,
    Dave
    

19. Re: Error on failed COMMIT

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-17T23:30:04Z

    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
    
    
    
    

20. Re: Error on failed COMMIT

    Haumacher, Bernhard <haui@haumacher.de> — 2020-02-20T07:02:49Z

    Am 17.02.2020 um 23:12 schrieb Dave Cramer:
    > On Mon, 17 Feb 2020 at 13:02, Haumacher, Bernhard <haui@haumacher.de 
    > <mailto: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
    even worse...
    >
    > 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.
    be free to choose/define one...
    >
    >     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
    
    

21. Re: Error on failed COMMIT

    Shay Rojansky <roji@roji.org> — 2020-02-23T05:40:58Z

    On Fri, 14 Feb 2020 at 14:37, Robert Haas <robertmhaas@gmail.com> wrote:
    >
    >> 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 we tend 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.
    >>
    >
    > Well con.commit() returns void :(
    >
    
    I'd like to second Dave on this, from the .NET perspective - actual client
    access is done via standard drivers in almost all cases, and these drivers
    generally adhere to database API abstractions (JDBC for Java, ADO.NET for
    .NET, and so on). AFAIK, in almost all such abstractions, commit can either
    complete (implying success) or throw an exception - there is 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 from the
    commit; this makes the mechanism very "undiscoverable".
    
    In other words, if we do agree that there are some legitimate cases where a
    program may end up executing commit on a failed transaction (e.g. because
    of a combination of framework and application code), and we think that a
    well-written client should be aware of the failed transaction and behave in
    an exceptional way around a non-committing commit, then I think that's a
    good case for a server-side change:
    
       - Asking drivers to do this at the client have the exact same breakage
       impact as the server change, since the user-visible behavior changes in the
       same way (the change is just shifted from server to driver). What's worse
       is that every driver 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).
       - Asking end-users (i.e. application code) to do this seems even worse,
       as every user/application in the world now has to be made somehow aware of
       a somewhat obscure and very un-discoverable situation.
    
    Shay
    

22. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-23T11:16:23Z

    On Sun, 23 Feb 2020 at 00:41, Shay Rojansky <roji@roji.org> wrote:
    
    >
    >
    > On Fri, 14 Feb 2020 at 14:37, Robert Haas <robertmhaas@gmail.com> wrote:
    >>
    >>> 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 we tend 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.
    >>>
    >>
    >> Well con.commit() returns void :(
    >>
    >
    > I'd like to second Dave on this, from the .NET perspective - actual client
    > access is done via standard drivers in almost all cases, and these drivers
    > generally adhere to database API abstractions (JDBC for Java, ADO.NET for
    > .NET, and so on). AFAIK, in almost all such abstractions, commit can either
    > complete (implying success) or throw an exception - there is 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 from the
    > commit; this makes the mechanism very "undiscoverable".
    >
    > In other words, if we do agree that there are some legitimate cases where
    > a program may end up executing commit on a failed transaction (e.g. because
    > of a combination of framework and application code), and we think that a
    > well-written client should be aware of the failed transaction and behave in
    > an exceptional way around a non-committing commit, then I think that's a
    > good case for a server-side change:
    >
    >    - Asking drivers to do this at the client have the exact same breakage
    >    impact as the server change, since the user-visible behavior changes in the
    >    same way (the change is just shifted from server to driver). What's worse
    >    is that every driver 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).
    >    - Asking end-users (i.e. application code) to do this seems even
    >    worse, as every user/application in the world now has to be made somehow
    >    aware of a somewhat obscure and very un-discoverable situation.
    >
    > Shay
    >
    
    To be fair this is Bernhard's position which, after thinking about this
    some more, I am endorsing.
    
    So we now have two of the largest client bases for PostgreSQL with known
    issues effectively losing data because they don't notice that the commit
    failed.
    It is very likely that this occurs with all clients but they just don't
    notice it. That is what is particularly alarming about this problem is that
    we are silently ignoring an error.
    
    While we can certainly code around this in the client drivers I don't
    believe they should be responsible for fixing the failings of the server.
    
    I fail to see where doing the right thing and reporting an error where
    there is one should be trumped by not breaking existing apps which by all
    accounts may be broken but just don't know it.
    
    Dave
    

23. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-23T12:14:25Z

    Shay> Asking drivers to do this at the client have the exact same breakage
    impact as the server change, since the user-visible behavior changes in the
    same way
    
    +1
    
    Dave>While we can certainly code around this in the client drivers I don't
    believe they should be responsible for fixing the failings of the server.
    
    Application developers expect that the database feels the same no matter
    which driver is used, so it would be better to avoid a case
    when half of the drivers create exceptions on non-committing-commit, and
    another half silently loses data.
    
    Vladimir
    

24. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-24T01:31:09Z

    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 almost all cases, and these drivers generally adhere to database API abstractions (JDBC for Java, ADO.NET for .NET, and so on). AFAIK, in almost all such abstractions, commit can either complete (implying success) or throw an exception - there is 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 from the 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-visible behavior changes in the same way (the change is just shifted from server to driver). What's worse is that every driver 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
    
    
    
    

25. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-24T01:58:59Z

    On Sun, 23 Feb 2020 at 20:31, Robert Haas <robertmhaas@gmail.com> wrote:
    
    > 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 almost all cases, and these
    > drivers generally adhere to database API abstractions (JDBC for Java,
    > ADO.NET for .NET, and so on). AFAIK, in almost all such abstractions,
    > commit can either complete (implying success) or throw an exception - there
    > is 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 from the 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.
    
    
    The driver does not ignore the error, But in Bernhard's case the framework
    is
    processing the exception and not re-throwing it.
    
    
    > 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.
    >
    
    Well the driver really isn't in the business of changing the semantics of
    the server.
    
    >
    > 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.
    >
    
    Yes, we could do that.
    
    >
    > 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.
    >
    
    We could also do this.
    
    >
    > > Asking drivers to do this at the client have the exact same breakage
    > impact as the server change, since the user-visible behavior changes in the
    > same way (the change is just shifted from server to driver). What's worse
    > is that every driver 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,
    
    
    I would argue that you really don't know if you had the problem or not
    since an error is not thrown.
    The client merrily goes along its way after issuing a commit and receiving
    a rollback or possibly  a warning
    saying that it's not in a transaction. One would have to know that the
    server
    had this behaviour to check for it.
    Clearly not everyone knows this as it's not documented as violation of the
    SQL spec
    
    
    > 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.
    
    
    Actually I would be willing to bet that the JDBC driver would do just that
    without any code changes whatsoever.
    commit throws an error. The driver sees the error and throws an exception.
    If an older version of the server were used no error would be thrown it
    wold work as seen today. I would also be willing to bet other drivers would
    work the same.
    Additionally once the server behaviour was changed I'd be more than willing
    to have the driver emulate this behaviour for older versions.
    
    
    
    > 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.
    
    Not seeing how this is a driver error.
    
    > 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,
    >
    It does throw the exception, however for whatever reason the client ignores
    these.
    Not how I code but apparently there is an application for this
    
    > 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.
    >
    
    I think the fact that this is a violation of the SQL SPEC lends
    considerable credence to the argument for changing the behaviour.
    Since this can lead to losing a transaction I think there is even more
    reason to look at changing the behaviour.
    
    Dave Cramer
    www.postgres.rocks
    

26. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2020-02-24T08:01:26Z

    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
    
    
    
    

27. Re: Error on failed COMMIT

    Shay Rojansky <roji@roji.org> — 2020-02-24T08:26:38Z

    > 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. [...]
    > I am really struggling to see why this is anything but a bug in the JDBC
    driver
    
    As Dave wrote, the problem here isn't with the driver, but with framework
    or user-code which swallows the initial exception and allows code to
    continue to the commit. Npgsql (and I'm sure the JDBC driver too) does
    surface PostgreSQL errors as exceptions, and internally tracks the
    transaction status provided in the CommandComplete message. That means
    users have the ability - but not the obligation - to know about failed
    transactions, and some frameworks or user coding patterns could lead to a
    commit being done on a failed transaction.
    
    > 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. [...]
    
    This is the main point here IMHO, and I don't think it's a question of
    convenience, or of behavior that should vary across drivers.
    
    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 I wrote above, from the user's perspective it makes no
    difference - the change would be identical (and just as breaking) either
    way. 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...).
    
    > 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 [...]
    
    If we are assuming that most user code is already written to avoid
    committing on failed transactions (by tracking transaction state etc.),
    then making this change at the server wouldn't affect those applications;
    the only applications affected would be those that do commit on failed
    transactions today, and it could be argued that those are likely to be
    broken today (since drivers today don't really expose the rollback in an
    accessible/discoverable way).
    
    Shay
    
    On Mon, Feb 24, 2020 at 3:31 AM Robert Haas <robertmhaas@gmail.com> wrote:
    
    > 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 almost all cases, and these
    > drivers generally adhere to database API abstractions (JDBC for Java,
    > ADO.NET for .NET, and so on). AFAIK, in almost all such abstractions,
    > commit can either complete (implying success) or throw an exception - there
    > is 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 from the 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-visible behavior changes in the
    > same way (the change is just shifted from server to driver). What's worse
    > is that every driver 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
    >
    

28. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-24T12:25:40Z

    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
    
    
    
    

29. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-24T12:29:39Z

    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
    
    
    
    

30. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-24T12:34:28Z

    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 exception and allows code to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does surface PostgreSQL errors as exceptions, and internally tracks the transaction status provided in the CommandComplete message. That means users have the ability - but not the obligation - to know about failed transactions, and some frameworks or user coding patterns could 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 I wrote above, from the user's perspective it makes no difference - the change would be identical (and just as breaking) either way. 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 problem 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 transaction state etc.), then making this change at the server wouldn't affect those applications; the only applications affected would be those that do commit on failed transactions today, and it could be argued that those are likely to be broken today (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
    
    
    
    

31. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-24T13:09:52Z

    On Mon, 24 Feb 2020 at 07:25, Robert Haas <robertmhaas@gmail.com> wrote:
    
    > 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.
    >
    > --
    >
    
    Fair enough. What I meant to say was that the driver isn't in the business
    of providing different semantics than the server provides.
    
    
    > Dave Cramer
    > http://www.postgres.rocks
    >
    

32. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-24T13:16:11Z

    On Mon, 24 Feb 2020 at 07:34, Robert Haas <robertmhaas@gmail.com> 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 exception and allows code
    > to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does
    > surface PostgreSQL errors as exceptions, and internally tracks the
    > transaction status provided in the CommandComplete message. That means
    > users have the ability - but not the obligation - to know about failed
    > transactions, and some frameworks or user coding patterns could lead to a
    > commit being done on a failed transaction.
    >
    > Agreed. All of that can be fixed in the driver, though.
    >
    
    Of course it can but we really don't want our users getting one experience
    with driver A and a different experience with driver B.
    
    >
    > > 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 I wrote above, from the user's perspective it makes no
    > difference - the change would be identical (and just as breaking) either
    > way. 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.
    >
    
    Sure it does. Any existing code that was relying on the existing semantics
    would be incompatible.
    
    
    >
    > > > 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 [...]
    > >
    > > If we are assuming that most user code is already written to avoid
    > committing on failed transactions (by tracking transaction state etc.),
    > then making this change at the server wouldn't affect those applications;
    > the only applications affected would be those that do commit on failed
    > transactions today, and it could be argued that those are likely to be
    > broken today (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.
    
    
    So how does one figure out what failed and what succeeded ? I would think
    it would be pretty difficult in a large sql script to go back and figure
    out what needed to be repaired. Seems to me it would be much easier if
    everything failed.
    
    
    > 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.
    >
    
    So the argument here is that we don't want to annoy some percentage of the
    population by doing the right thing ?
    
    
    
    Dave Cramer
    www.postgres.rocks
    

33. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-24T13:21:48Z

    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.
    >
    
    To test how the driver would currently react if the server did respond with
    an error I made a small change
    
    diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
    index 0a6f80963b..9405b0cfd9 100644
    --- a/src/backend/tcop/postgres.c
    +++ b/src/backend/tcop/postgres.c
    @@ -2666,8 +2666,7 @@ IsTransactionExitStmt(Node *parsetree)
            {
                    TransactionStmt *stmt = (TransactionStmt *) parsetree;
    
    -               if (stmt->kind == TRANS_STMT_COMMIT ||
    -                       stmt->kind == TRANS_STMT_PREPARE ||
    +               if (stmt->kind == TRANS_STMT_PREPARE ||
                            stmt->kind == TRANS_STMT_ROLLBACK ||
                            stmt->kind == TRANS_STMT_ROLLBACK_TO)
                            return true;
    
    I have no idea how badly this breaks other things but it does throw an
    error on commit if the transaction is in error.
    With absolutely no changes to the driver this code does what I would expect
    and executes the conn.rollback()
    
    try {
        conn.setAutoCommit(false);
        try {
            conn.createStatement().execute("insert into notnullable values (NULL)");
        } catch (SQLException ex ) {
            ex.printStackTrace();
            //ignore this exception
        }
        conn.commit();
    } catch ( SQLException ex ) {
        ex.printStackTrace();
        conn.rollback();
    }
    conn.close();
    
    Dave Cramer
    
    http://www.postgres.rocks
    
    
    
    >
    

34. Re: Error on failed COMMIT

    Shay Rojansky <roji@roji.org> — 2020-02-24T13:24:19Z

    >> 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 I wrote above, from the user's perspective it makes no
    difference - the change would be identical (and just as breaking) either
    way. 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.
    
    I think it's very important to expand the reasoning here from "server and
    client" to "server, drivers, users". As I wrote above, changing this
    behavior in a driver is just as much a compatibility break for any user of
    that driver, as a server change; it's true that PostgreSQL would not be
    "responsible" ot "at fault" but rather the driver writer, but as far as
    angry users go there's very little difference. A break is a break, whether
    it happens because of a PostgreSQL change, or because of a .NET/Java driver
    change.
    
    > 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.
    
    As above, if Dave and I make this change in the JDBC driver and/or Npgsql,
    all applications relying on the previous behavior would be just as broken.
    
    >> If we are assuming that most user code is already written to avoid
    committing on failed transactions (by tracking transaction state etc.),
    then making this change at the server wouldn't affect those applications;
    the only applications affected would be those that do commit on failed
    transactions today, and it could be argued that those are likely to be
    broken today (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.
    
    The question is more whether typical user applications are actually
    checking for rollback-on-commit, not whether they theoretically can. An
    exception is something you have to actively swallow to ignore; an
    additional returned status saying "hey, this didn't actually commit" is
    extremely easy to ignore unless you've specifically been aware of the
    situation.
    
    Even so, a quick look at psycopg and Ruby (in addition to JDBC and .NET),
    commit APIs generally don't return anything - this is just how the API
    abstractions are, probably because across databases nothing like that is
    needed (the expectation is for a non-throwing commit to imply that the
    commit occurred).
    
    Shay
    
    On Mon, Feb 24, 2020 at 2:34 PM Robert Haas <robertmhaas@gmail.com> 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 exception and allows code
    > to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does
    > surface PostgreSQL errors as exceptions, and internally tracks the
    > transaction status provided in the CommandComplete message. That means
    > users have the ability - but not the obligation - to know about failed
    > transactions, and some frameworks or user coding patterns could 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 I wrote above, from the user's perspective it makes no
    > difference - the change would be identical (and just as breaking) either
    > way. 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 problem 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 transaction state etc.),
    > then making this change at the server wouldn't affect those applications;
    > the only applications affected would be those that do commit on failed
    > transactions today, and it could be argued that those are likely to be
    > broken today (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
    >
    

35. Re: Error on failed COMMIT

    David Fetter <david@fetter.org> — 2020-02-24T17:37:47Z

    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 exception and allows code to continue to the commit. Npgsql (and I'm sure the JDBC driver too) does surface PostgreSQL errors as exceptions, and internally tracks the transaction status provided in the CommandComplete message. That means users have the ability - but not the obligation - to know about failed transactions, and some frameworks or user coding patterns could 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 I wrote above, from the user's perspective it makes no difference - the change would be identical (and just as breaking) either way. 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 problem 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 transaction state etc.), then making this change at the server wouldn't affect those applications; the only applications affected would be those that do commit on failed transactions today, and it could be argued that those are likely to be broken today (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
    
    
    
    

36. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2020-02-24T17:40:16Z

    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
    
    
    
    

37. Re: Error on failed COMMIT

    David Fetter <david@fetter.org> — 2020-02-24T17:53:34Z

    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
    
    
    
    

38. Re: Error on failed COMMIT

    Merlin Moncure <mmoncure@gmail.com> — 2020-02-24T19:58:00Z

    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 the behaviour.
    > 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
    
    
    
    

39. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-24T22:06:23Z

    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, the transaction should be
    terminated.
    COMMIT
    
    No-one on the thread suggests the transaction must hang forever.
    Of course, commit must terminate the transaction one way or another.
    The proposed change is to surface the exception if user tries to commit or
    prepare a transaction that can't be committed.
    Note: the reason does not matter much. If deferred constraint fails on
    commit, then commit itself throws an error.
    Making commit throw an error in case "current transaction is aborted" makes
    perfect sense.
    
    Note: the same thing is with PREPARE TRANSACTION 'txname`.
    Apparently it silently responses with ROLLBACK which is strange as well.
    
    Vladimir
    

40. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2020-02-24T22:37:45Z

    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
    
    
    
    

41. Re: Error on failed COMMIT

    Merlin Moncure <mmoncure@gmail.com> — 2020-02-24T22:59:37Z

    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, the transaction 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
    
    
    
    

42. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-24T23:22:21Z

    On Mon, 24 Feb 2020 at 17:59, Merlin Moncure <mmoncure@gmail.com> wrote:
    
    > 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, the transaction 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?
    >
    
    Don't see why not. All that happens is that an error message is emitted by
    the server on commit instead of silently rolling back
    
    
    Dave Cramer
    https://www.postgres.rocks
    

43. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-25T05:58:43Z

    >do you think most common connection poolers would continue to
    >work after making this change?
    
    Of course, they should.
    There are existing cases when commit responds with an error: deferrable
    constraints.
    
    There's nothing new except it is suggested to make the behavior of
    commit/prepare failure (e.g. "can't commit the transaction because...")
    consistent with other commit failures (e.g. deferred violation).
    
    Vladimir
    

44. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-25T06:45:39Z

    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 server provides.
    
    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
    
    
    
    

45. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-25T07:17:40Z

    Robert>Now, of course, it's also true that if what the server does makes
    Robert>users sad, maybe the server should do something different
    
    The server makes users sad as it reports the same end result (=="commit
    failed") differently.
    Sometimes the server produces ERROR, and sometimes the server produces "OK,
    the transaction was rolled back".
    
    The users do expect that commit might fail, and they don't really expect
    that sometimes commit can be silently converted to a rollback.
    
    Robert>BEGIN;
    Robert>-- do stuff
    Robert>COMMIT;
    Robert>BEGIN;
    Robert>-- do more stuff
    Robert>COMMIT;
    
    Robert>...and they run these scripts by piping them into psql. Now, if the
    Robert>COMMIT leaves the session in a transaction state,
    
    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 rather than silently doing a rollback.
    
    Vladimir
    

46. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-25T07:55:03Z

    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 rather than 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
    
    
    
    

47. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-25T08:42:47Z

    Tom>I think we still end up concluding that altering this behavior has more
    Tom>downside than upside.
    
    What is the downside?
    
    Applications, drivers, and poolers already expect that commit might produce
    an error and terminate the transaction at the same time.
    
    "The data is successfully committed to the database if and only if commit
    returns without error".
    ^^^ the above is way easier to reason about than "user must check multiple
    unrelated outcomes to tell if the changes are committed or not".
    
    Vladimir
    

48. Re: Error on failed COMMIT

    Laurenz Albe <laurenz.albe@cybertec.at> — 2020-02-25T11:11:21Z

    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 rather than 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
    
    
    
    
    

49. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-26T18:23:25Z

    Just one more data point: drivers do allow users to execute queries in a
    free form.
    Shat is the user might execute /*comment*/commit/*comment*/ as a free-form
    SQL, and they would expect that the resulting
     behaviour should be exactly the same as .commit() API call (==silent
    rollback is converted to an exception).
    
    That is drivers can add extra logic into .commit() API implementation,
    however, turning free-form SQL into exceptions
    is hard to do consistently from the driver side.
    It is not like "check the response from .commit() result".
    It is more like "don't forget to parse user-provided SQL and verify if it
    is semantically equivalent to commit"
    
    Pushing full SQL parser to the driver is not the best idea taking into the
    account the extensibility the core has.
    
    Vladimir
    

50. Re: Error on failed COMMIT

    Haumacher, Bernhard <haui@haumacher.de> — 2020-02-26T18:33:56Z

    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
    
    
    
    
    
    

51. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2020-02-26T18:46:33Z

    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 rather than 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
    
    
    
    

52. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-26T20:51:39Z

    On Wed, 26 Feb 2020 at 13:46, Vik Fearing <vik@postgresfriends.org> wrote:
    
    > 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 rather than 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
    >
    
    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 ?
    What it does is create another server error level that will emit the error
    and return as opposed to not returning.
    I honestly haven't given much thought to the error message. At this point I
    just want the nod as to how to do it.
    

53. Re: Error on failed COMMIT

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-26T21:22:59Z

    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
    
    
    
    

54. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2020-02-26T21:57:19Z

    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
    
    
    
    

55. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-26T22:11:56Z

    On Wed, 26 Feb 2020 at 16:57, Vik Fearing <vik@postgresfriends.org> wrote:
    
    > 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.
    
    
    I'm not convinced of this. I doubt we actually have any real numbers?
    
    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.
    >
    Clients can code around this as well for old servers. This is something
    that is more palatable
    if the server defines this behaviour.
    
    >
    > > 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.
    >
    
    I think his point is that the error is emitted before we actually do the
    rollback and it could fail.
    
    
    >
    > 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.
    >
    
    I guess the error has to be sent after the rollback completes.
    
    >
    > 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.
    >
    
    Pretty sure I can code around this.
    
    -- 
    > Vik Fearing
    >
    

56. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-02-27T04:04:34Z

    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
    
    
    
    

57. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-02-27T04:11:26Z

    But if the SQL is /*commit*/rollback, then the driver should not raise an
    exception. The exception should be only for the case when the client asks
    to commit and the database can't do that.
    
    The resulting command tag alone is not enough.
    
    Vladimir
    

58. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-27T12:44:14Z

    On Wed, 26 Feb 2020 at 16:57, Vik Fearing <vik@postgresfriends.org> wrote:
    
    > 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.
    >
    
    Ok, here is a much less obtrusive solution thanks to Vladimir.
    
    FWIW, only 10 of 196 tests fail.
    Dave Cramer
    www.postgres.rocks
    
    >
    >
    

59. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-02-27T16:30:17Z

    On Thu, 27 Feb 2020 at 07:44, Dave Cramer <davecramer@postgres.rocks> wrote:
    
    >
    >
    >
    > On Wed, 26 Feb 2020 at 16:57, Vik Fearing <vik@postgresfriends.org> wrote:
    >
    >> 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.
    >>
    >
    > Ok, here is a much less obtrusive solution thanks to Vladimir.
    >
    
    Still had to mess with error levels since commit and chain needs the
    existing context to succeed.
    
    After fixing up the tests only 1 still failing.
    
    
    Dave Cramer
    http://www.postgres.rocks
    

60. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-03-06T16:54:51Z

    On Thu, 27 Feb 2020 at 08:30, Dave Cramer <davecramer@postgres.rocks> wrote:
    
    >
    >
    > On Thu, 27 Feb 2020 at 07:44, Dave Cramer <davecramer@postgres.rocks>
    > wrote:
    >
    >>
    >>
    >>
    >> On Wed, 26 Feb 2020 at 16:57, Vik Fearing <vik@postgresfriends.org>
    >> wrote:
    >>
    >>> 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.
    >>>
    >>
    >> Ok, here is a much less obtrusive solution thanks to Vladimir.
    >>
    >
    > Still had to mess with error levels since commit and chain needs the
    > existing context to succeed.
    >
    > After fixing up the tests only 1 still failing.
    >
    
    
    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.
    
    however if the client executes commit using direct SQL and possibly
    multiplexes a number of commands we would have to parse the SQL to figure
    out what is being sent. This could include a column named commit_date or a
    comment with commit embedded in it. It really doesn't make sense to have a
    full fledged PostgreSQL SQL parser in every client. This is something the
    server does very well.
    
    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.
    
    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 type of 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 it could make sense, rather
    than wiring in assumptions about the exact sequence of messages.
    
    Seems to me that this behaviour is already documented?
    
    Dave Cramer
    http://www.postgres.rocks
    
    
    
    >
    

61. Re: Error on failed COMMIT

    Robert Haas <robertmhaas@gmail.com> — 2020-03-06T18:12:10Z

    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 parse the SQL to figure out what is being sent. This could include a column named commit_date or a comment with commit embedded in it. It really doesn't make sense to have a full fledged PostgreSQL SQL parser in every client. This is something 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.
    
    > 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 type of 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 it could 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
    
    
    
    

62. Re: Error on failed COMMIT

    Bruce Momjian <bruce@momjian.us> — 2020-03-17T20:47:41Z

    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 to parse the SQL to figure out what is being sent. This could include a column named commit_date or a comment with commit embedded in it. It really doesn't make sense to have a full fledged PostgreSQL SQL parser in every client. This is something 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 +
    
    
    
    

63. Re: Error on failed COMMIT

    Vladimir Sitnikov <sitnikov.vladimir@gmail.com> — 2020-03-17T21:22:22Z

    Bruce, thanks for taking the time to summarize.
    
    Bruce>Fourth, it is not clear how many applications would break if COMMIT
    Bruce>started issuing an error rather than return success
    
    None.
    
    Bruce>applications that issue COMMIT and expect success after a transaction
    Bruce>block has failed
    
    An application must expect an exception from a COMMIT statement like any
    other SQL.
    
    Wire protocol specification explicitly says implementations must expect
    error messages at any time.
    
    ---
    
    Bruce>Do we know how other database systems handle this?
    
    Oracle DB produces an error from COMMIT if transaction can't be committed
    (e.g. failure in the processing of "on commit refresh materialized view").
    
    ---
    
    The bug is "deferred constraint violation" and "non-deferred constraint
    violation" end up with
    **different** behavior for COMMIT.
    
    deferred violation produces an error while non-deferred violation produces
    "silent rollback".
    
    In other words, there are already cases in PostgreSQL when commit produces
    an error. It is nothing new.
    The new part is that PostgreSQL must not produce "silent rollbacks".
    
    Bruce>First, Vik reported that we don't follow the SQL spec
    
    +1
    
    Bruce>Second, someone suggested that if COMMIT throws an error, that future
    Bruce>statements would be considered to be in the same transaction
    
    No. Please disregard that. That is ill. COMMIT (and/or ROLLBACK) must
    terminate the transaction in any case.
    The transaction must not exist after COMMIT finishes (successfully or not).
    The same for PREPARE TRANSACTION. If it fails, then the transaction must be
    clear.
    
    A litmus test is "deferred constraint violation". It works Ok in the
    current PostgreSQL.
    If the database can't commit, it should respond with a clear error that
    describes the reason for the failure.
    
    Bruce>Third, the idea that individual interfaces, e.g. JDBC, should throw
    
    Individual interfaces should not deviate from server behavior much.
    They should convert server-provided errors to the language-native format.
    They should not invent their own rules to convert server messages to errors.
    That would provide a uniform PostgreSQL experience for the end-users.
    
    Note: there are even multiple JDBC implementations for PostgreSQL, so
    slight differences in transaction handling
    is the very last "feature" people want from PostgreSQL database.
    
    Vladimir
    

64. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-03-17T23:15:05Z

    On Tue, 17 Mar 2020 at 16:47, Bruce Momjian <bruce@momjian.us> wrote:
    
    > 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 to parse the SQL to figure
    > out what is being sent. This could include a column named commit_date or a
    > comment with commit embedded in it. It really doesn't make sense to have a
    > full fledged PostgreSQL SQL parser in every client. This is something 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.)
    >
    
    Good question.
    
    >
    > 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.
    >
    > This is correct. The patch I provided does exactly this.
    The Rollback occurs. The transaction is finished, but an error message is
    sent
    
    
    > 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.
    
    >
    > 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.
    
    I suspect npgsql would be fine. Shay ?
    
    
    Dave Cramer
    www.postgres.rocks
    

65. Re: Error on failed COMMIT

    Bruce Momjian <bruce@momjian.us> — 2020-03-17T23:23:30Z

    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 +
    
    
    
    

66. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-03-17T23:32:00Z

    On Tue, 17 Mar 2020 at 19:23, Bruce Momjian <bruce@momjian.us> wrote:
    
    > 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.
    >
    > Agreed and this is why I think it is important for the server to be
    defining the behaviour instead of each interface deciding how to handle
    this situation.
    
    
    Dave Cramer
    www.postgres.rocks
    
    >
    >
    

67. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-03-30T16:05:03Z

    On Tue, 17 Mar 2020 at 19:32, Dave Cramer <davecramer@postgres.rocks> wrote:
    
    >
    >
    > On Tue, 17 Mar 2020 at 19:23, Bruce Momjian <bruce@momjian.us> wrote:
    >
    >> 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.
    >>
    >> Agreed and this is why I think it is important for the server to be
    > defining the behaviour instead of each interface deciding how to handle
    > this situation.
    >
    >
    >
    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 ?
    
    Dave Cramer
    www.postgres.rocks
    

68. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2020-03-30T16:32:16Z

    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
    
    
    
    

69. Re: Error on failed COMMIT

    Shay Rojansky <roji@roji.org> — 2020-03-30T21:06:39Z

    Apologies for not responding earlier, busy times.
    
    
    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.
    >
    > I suspect npgsql would be fine. Shay ?
    >
    
    Npgsql would be fine. In fact, Npgsql doesn't have any specific
    expectations nor any specific logic around commit; it assumes errors may be
    returned for any command (COMMIT or otherwise), and surfaces those errors
    as .NET exceptions. The transaction status is tracked via CommandComplete
    only, and as mentioned several times, PostgreSQL can already error on
    commit for various other reasons (e.g. deferred constraint checks). This
    direction makes a lot of sense to me.
    

70. Re: Error on failed COMMIT

    Tony Locke <tlocke@tlocke.org.uk> — 2020-04-18T15:27:55Z

    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 assumes errors 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.
    
    
    
    

71. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-08-04T16:19:27Z

    Attached is the rebased patch for consideration.
    
    Dave Cramer
    www.postgres.rocks
    
    
    
    >
    >
    

72. Re: Error on failed COMMIT

    Andrew Dunstan <andrew.dunstan@2ndquadrant.com> — 2020-09-30T22:14:52Z

    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
    
    
    
    
    

73. Re: Error on failed COMMIT

    Georgios Kokolatos <gkokolatos@protonmail.com> — 2020-10-30T16:13:13Z

    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

74. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-11-09T21:26:59Z

    On Wed, 30 Sep 2020 at 18:14, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
    wrote:
    
    >
    > 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
    >
    
    
    Attached is a rebased patch with fixes for the isolation tests
    
    
    Dave Cramer
    www.postgres.rocks
    

75. Re: Error on failed COMMIT

    Georgios Kokolatos <gkokolatos@protonmail.com> — 2020-11-10T15:19:53Z

    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
    

76. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2020-11-10T16:53:20Z

    On Mon, 9 Nov 2020 at 16:26, Dave Cramer <davecramer@postgres.rocks> wrote:
    
    >
    >
    > On Wed, 30 Sep 2020 at 18:14, Andrew Dunstan <
    > andrew.dunstan@2ndquadrant.com> wrote:
    >
    >>
    >> 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
    >>
    >
    >
    > Attached is a rebased patch with fixes for the isolation tests
    >
    >
    
    >
    > Dave Cramer
    > www.postgres.rocks
    >
    

77. Re: Error on failed COMMIT

    Georgios Kokolatos <gkokolatos@protonmail.com> — 2020-12-01T09:48:08Z

    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
    

78. Re: Error on failed COMMIT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-01-07T14:26:58Z

    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/
    
    
    
    

79. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-07T14:29:12Z

    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.
    
    Dave Cramer
    www.postgres.rocks
    
    
    On Thu, 7 Jan 2021 at 09:27, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
    
    > 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/
    >
    

80. Re: Error on failed COMMIT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-01-22T14:27:35Z

    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/
    
    
    
    

81. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-25T14:09:09Z

    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.
    
    Regards,
    
    Dave Cramer
    www.postgres.rocks
    
    
    On Tue, 10 Nov 2020 at 11:53, Dave Cramer <davecramer@postgres.rocks> wrote:
    
    >
    >
    > On Mon, 9 Nov 2020 at 16:26, Dave Cramer <davecramer@postgres.rocks>
    > wrote:
    >
    >>
    >>
    >> On Wed, 30 Sep 2020 at 18:14, Andrew Dunstan <
    >> andrew.dunstan@2ndquadrant.com> wrote:
    >>
    >>>
    >>> 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
    >>>
    >>
    >>
    >> Attached is a rebased patch with fixes for the isolation tests
    >>
    >>
    >
    >>
    >> Dave Cramer
    >> www.postgres.rocks
    >>
    >
    

82. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-25T16:29:10Z

    Apologies, I should have checked again to make sure the patch applied.
    
    This one does and passes tests.
    
    Dave Cramer
    www.postgres.rocks
    
    
    On Mon, 25 Jan 2021 at 09:09, Dave Cramer <davecramer@postgres.rocks> 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.
    >
    > Regards,
    >
    > Dave Cramer
    > www.postgres.rocks
    >
    >
    > On Tue, 10 Nov 2020 at 11:53, Dave Cramer <davecramer@postgres.rocks>
    > wrote:
    >
    >>
    >>
    >> On Mon, 9 Nov 2020 at 16:26, Dave Cramer <davecramer@postgres.rocks>
    >> wrote:
    >>
    >>>
    >>>
    >>> On Wed, 30 Sep 2020 at 18:14, Andrew Dunstan <
    >>> andrew.dunstan@2ndquadrant.com> wrote:
    >>>
    >>>>
    >>>> 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
    >>>>
    >>>
    >>>
    >>> Attached is a rebased patch with fixes for the isolation tests
    >>>
    >>>
    >>
    >>>
    >>> Dave Cramer
    >>> www.postgres.rocks
    >>>
    >>
    

83. Re: Error on failed COMMIT

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-01-26T10:05:53Z

    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
    
    
    
    
    

84. Re: Error on failed COMMIT

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-01-26T11:58:59Z

    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/
    
    
    
    

85. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-26T13:43:41Z

    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/
    >
    

86. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-26T16:09:12Z

    On Tue, 26 Jan 2021 at 05:05, 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.
    >
    > 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
    
    >
    >
    

87. Re: Error on failed COMMIT

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-01-26T17:20:41Z

    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
    
    
    
    
    

88. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-26T17:25:57Z

    On Tue, 26 Jan 2021 at 12:20, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
    
    > 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.
    >
    
    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 ?
    
    Dave
    
    
    >
    

89. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2021-01-26T17:34:34Z

    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
    
    
    
    

90. Re: Error on failed COMMIT

    Vik Fearing <vik@postgresfriends.org> — 2021-01-26T17:37:47Z

    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
    
    
    
    

91. Re: Error on failed COMMIT

    Laurenz Albe <laurenz.albe@cybertec.at> — 2021-01-26T17:46:10Z

    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
    
    
    
    
    

92. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-01-26T18:02:06Z

    On Tue, 26 Jan 2021 at 12:46, Laurenz Albe <laurenz.albe@cybertec.at> wrote:
    
    > 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...
    >
    
    OK, let me look into how we deal with COMMIT and CHAIN.
    
    I can see some real issues with this as Vik pointed out.
    
    Dave
    

93. Re: Error on failed COMMIT

    David Steele <david@pgmasters.net> — 2021-03-25T16:04:44Z

    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
    
    
    
    

94. Re: Error on failed COMMIT

    Dave Cramer <davecramer@postgres.rocks> — 2021-03-25T19:07:17Z

    On Thu, 25 Mar 2021 at 12:04, David Steele <david@pgmasters.net> wrote:
    
    > 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?
    >
    >
    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.
    
    
    Dave Cramer
    www.postgres.rocks
    
    > Regards,
    > --
    > -David
    > david@pgmasters.net
    >
    

95. Re: Error on failed COMMIT

    David Steele <david@pgmasters.net> — 2021-03-25T19:23:53Z

    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