Error on failed COMMIT

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?)

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.

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

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.

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. 

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"?

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).