Error on failed COMMIT

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
92 messages Options
12345
Reply | Threaded
Open this post in threaded view
|

Error on failed COMMIT

Vik Fearing-6
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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Tom Lane-2
Vik Fearing <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Vik Fearing-6
On 11/02/2020 23:35, Tom Lane wrote:

> Vik Fearing <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Tom Lane-2
Vik Fearing <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-4
In reply to this post by Tom Lane-2


On Tue, 11 Feb 2020 at 17:35, Tom Lane <[hidden email]> wrote:
Vik Fearing <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Haumacher, Bernhard
In reply to this post by Tom Lane-2
Am 12.02.2020 um 00:27 schrieb Tom Lane:

> Vik Fearing <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Robert Haas
On Thu, Feb 13, 2020 at 2:38 AM Haumacher, Bernhard <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-7



On Fri, 14 Feb 2020 at 12:37, Robert Haas <[hidden email]> wrote:
On Thu, Feb 13, 2020 at 2:38 AM Haumacher, Bernhard <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Robert Haas
On Fri, Feb 14, 2020 at 1:04 PM Dave Cramer <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-7


On Fri, 14 Feb 2020 at 13:29, Robert Haas <[hidden email]> wrote:
On Fri, Feb 14, 2020 at 1:04 PM Dave Cramer <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Robert Haas
On Fri, Feb 14, 2020 at 2:08 PM Dave Cramer <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-7


On Fri, 14 Feb 2020 at 14:37, Robert Haas <[hidden email]> wrote:
On Fri, Feb 14, 2020 at 2:08 PM Dave Cramer <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Tom Lane-2
Dave Cramer <[hidden email]> writes:
> On Fri, 14 Feb 2020 at 14:37, Robert Haas <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-7



On Fri, 14 Feb 2020 at 15:07, Tom Lane <[hidden email]> wrote:
Dave Cramer <[hidden email]> writes:
> On Fri, 14 Feb 2020 at 14:37, Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-7


On Fri, 14 Feb 2020 at 15:19, Alvaro Herrera <[hidden email]> 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
 
Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Haumacher, Bernhard
In reply to this post by Robert Haas
Am 14.02.2020 um 20:36 schrieb Robert Haas:

> On Fri, Feb 14, 2020 at 2:08 PM Dave Cramer <[hidden email]> 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 <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Dave Cramer-7



On Mon, 17 Feb 2020 at 13:02, Haumacher, Bernhard <[hidden email]> wrote:
Am 14.02.2020 um 20:36 schrieb Robert Haas:
> On Fri, Feb 14, 2020 at 2:08 PM Dave Cramer <[hidden email]> 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 <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Alvaro Herrera-9
In reply to this post by Dave Cramer-7
On 2020-Feb-14, Dave Cramer wrote:

> On Fri, 14 Feb 2020 at 15:19, Alvaro Herrera <[hidden email]>
> 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


Reply | Threaded
Open this post in threaded view
|

Re: Error on failed COMMIT

Haumacher, Bernhard
In reply to this post by Dave Cramer-7
Am 17.02.2020 um 23:12 schrieb Dave Cramer:
On Mon, 17 Feb 2020 at 13:02, Haumacher, Bernhard <[hidden email]> 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

12345