Обсуждение: commit and rollback don't throw exceptions when they should

Based on this:

http://java.sun.com/javase/6/docs/api/java/sql/Connection.html#commit%28%29

Connection.commit() and Connetion.rollback() in the PostgreSQL JDBC
driver don't throw exceptions when they should.  This was posted as
bug 5127 on the bugs list when it broke a user's connection pooling.
Are there any arguments that the PostgreSQL driver should not throw
and exception as stated on that javadoc page?:

- if a database access error occurs,
- this method is called while participating in a distributed
  transaction,
- if this method is called on a closed connection or
- this Connection object is in auto-commit mode

I could take a shot at a patch if nobody disagrees with making this
change.

-Kevin

Kevin Grittner wrote:

> I could take a shot at a patch if nobody disagrees with making this
> change.

What exactly is the change you're suggesting?

-O

Oliver Jowett <oliver@opencloud.com> wrote:
> Kevin Grittner wrote:
>
>> I could take a shot at a patch if nobody disagrees with making this
>> change.
>
> What exactly is the change you're suggesting?

I need to go back and see if any earlier versions conflict with this,
but recent version of the Java API specify that commit and rollback
should throw a SQLException if autoCommit is true.  The driver
currently returns without any warning or exception, and without doing
anything.

There is a similar requirement for these methods when a distributed
transaction is active, which is only mentioned in Java 6 javadocs, but
seems as though it makes sense whenever distributed transactions are
used.  Java 6 also explicitly mentions that an exception should be
thrown for attempts to use these methods on a closed connection,
although that could arguably fall under the earlier language requiring
an exception when "a database access error occurs" -- since you can't
access a database across a closed connection.

This surfaced because a popular connection pooler is reportedly
failing to behave correctly when the connection is lost (for example,
if PostgreSQL is restarted).  It works with other databases the user
has tested.  The user believes it to be because the rollback method is
not throwing an exception on the closed connection.  I haven't
independently confirmed this yet.  (Takiguchi, if you have a
self-contained test case, that would save me some time on that point.)

I'm suggesting that we throw exceptions for all of the above.  It
worries me that some existing applications may be relying on the
silent success of a commit or rollback when autoCommit is active,
though.  That's contrary to the javadocs for the API, but there's
still the backwards compatibility issue for current PostgreSQL
users....

-Kevin

Kevin Grittner wrote:

> I need to go back and see if any earlier versions conflict with this,
> but recent version of the Java API specify that commit and rollback
> should throw a SQLException if autoCommit is true.  The driver
> currently returns without any warning or exception, and without doing
> anything.

Sounds reasonable.

> Java 6 also explicitly mentions that an exception should be
> thrown for attempts to use these methods on a closed connection,
> although that could arguably fall under the earlier language requiring
> an exception when "a database access error occurs" -- since you can't
> access a database across a closed connection.

Taking a quick look at the code, the driver should already throw an
exception on closed connection if any communication with the server is
actually needed (if autocommit is on or if no commands have been
executed in the current transaction, then it doesn't do any
communication and you won't get an exception). Putting an explicit check
for a closed connection wouldn't hurt.

-O

taktos wrote:
> I made a groovy code that tests whether Connection#commit() throws
> excepton when connection has been closed.
>
> http://github.com/taktos/JDBC-Driver-Test/blob/master/ConnectionTest.groovy
>
> I tested in environment below:
> Oracle 10g XE (ojdbc14.jar and ojdbc6.jar)
> MySQL 5.1 (mysql-connector-java-5.1.10.jar)
> h2database (h2-1.2.121.jar)
> PostgreSQL 8.3 (postgresql-8.3.605.jdbc4.jar)
>
> Drivers other than PosrgreSQL throws exception as I had expected, but
> PostgreSQL didn't throw exception.

Yes, that is the case I mentioned where there have been no commands
executed in the transaction. In that case, the driver has no work to do
on commit() so it does not notice the connection was closed.

Even if we added a check for an explicit Connection.close() call before
Connection.commit(), that would not help with your test where you are
shutting down the server. The driver will only notice the network
connection is gone when it next tries to actually send or receive data.

Try adding a simple query ("SELECT 1", or whatever) to your test, and
you should see an exception on close.

-O

Oliver Jowett <oliver@opencloud.com> wrote:

>> Drivers other than PosrgreSQL throws exception as I had expected,
>> but PostgreSQL didn't throw exception.
>
> Yes, that is the case I mentioned where there have been no commands
> executed in the transaction. In that case, the driver has no work to
> do on commit() so it does not notice the connection was closed.
>
> Even if we added a check for an explicit Connection.close() call
> before Connection.commit(), that would not help with your test where
> you are shutting down the server. The driver will only notice the
> network connection is gone when it next tries to actually send or
> receive data.

A connection is allowed to be flagged as closed if a fatal error is
found; and in fact it seems that the PostgreSQL JDBC driver does this.
With the attached WIP patch (comments and documentation will need
attention, if nothing else), the problem seems corrected, based on
this Java reworking of groovy script.

import java.sql.*;
public final class ConnectionTest
{
    static final String DRIVER_CLASS_NAME = "org.postgresql.Driver";
    static final String URL = "jdbc:postgresql:test?user=kgrittn";
    public static void main(String[] args) throws Exception {
        Class driverClass = Class.forName(DRIVER_CLASS_NAME);
        Connection con = DriverManager.getConnection(URL);
        con.setAutoCommit(false);
        System.out.print("Restart the DB server and press any key.");
        System.in.read();
        PreparedStatement ps =
            con.prepareStatement("insert into foo values (?)");
        ps.setInt(1, 1);
        try {
            ps.executeUpdate();
        } catch (SQLException e) {
            e.printStackTrace();
            try {
                System.out.println("Attempting rollback.");
                con.rollback();
            } catch (SQLException rollbackFailed) {
                rollbackFailed.printStackTrace();
            }
        }
        con.close();
    }
}

With or without a restart I get no exception on the rollback attempt
following the failure of the ps.executeUpdate() line.  With this
patch, the rollback attempt throws this:

org.postgresql.util.PSQLException: Cannot rollback when connection is
closed.
        at
org.postgresql.jdbc2.AbstractJdbc2Connection.rollback(
AbstractJdbc2Connection.java:710)
        at ConnectionTest.main(ConnectionTest.java:29)

I think this should solve the problem for any reasonable pooler
implementation.  Does anyone disagree or see a problem with this
approach?  (If not, I'll try to polish it up.)

-Kevin

I made a groovy code that tests whether Connection#commit() throws
excepton when connection has been closed.

http://github.com/taktos/JDBC-Driver-Test/blob/master/ConnectionTest.groovy

I tested in environment below:
Oracle 10g XE (ojdbc14.jar and ojdbc6.jar)
MySQL 5.1 (mysql-connector-java-5.1.10.jar)
h2database (h2-1.2.121.jar)
PostgreSQL 8.3 (postgresql-8.3.605.jdbc4.jar)

Drivers other than PosrgreSQL throws exception as I had expected, but
PostgreSQL didn't throw exception.

Regards,
takiguchi

taktos <taktos@gmail.com> wrote:

> Drivers other than PosrgreSQL throws exception as I had expected,
> but PostgreSQL didn't throw exception.

Having received no feedback on the preliminary patch I previously
posted, I'll suggest this (fairly minimal) patch.  I didn't attempt to
deal with the distributed transaction issue, since it seemed pretty
messy and I wasn't sure the fix was worth the mess it would make.  As
far as I can see, it should fix the OP's problem, although it would be
nice to get a confirmation of that.

This patch is basically the same as the previous one, except that I've
updated the javadocs to match the new behavior.

If there are any issues with this, I can have another go at it.

-Kevin

On Wed, 4 Nov 2009, Kevin Grittner wrote:

> Having received no feedback on the preliminary patch I previously
> posted, I'll suggest this (fairly minimal) patch.  I didn't attempt to
> deal with the distributed transaction issue, since it seemed pretty
> messy and I wasn't sure the fix was worth the mess it would make.  As
> far as I can see, it should fix the OP's problem, although it would be
> nice to get a confirmation of that.
>

I've commited a modified version of this patch to HEAD only, because
changing this behavior could break peoples applications.

I extended the check for the connection already being closed to the other
connection methods.  There's no reason commit/rollback are special here.
I also needed to fix a regression testcase which was violating this rule.

Kris Jurka

Kris Jurka <books@ejurka.com> wrote:

> I've commited a modified version of this patch to HEAD only,
> because changing this behavior could break peoples applications.
>
> I extended the check for the connection already being closed to the
> other connection methods.  There's no reason commit/rollback are
> special here.

That all makes sense.  I think you have a copy/paste error in the
exception message, though: "This statement has been closed."  It is
the connection which has been closed here.

Also, for software which tries to make sense of the SQLSTATE for
automated handling, I'm inclined to think that something in Class Code
'08' (Connection Exception) would be more useful than the Class Code
'55' (Object Not in Prerequisite State) value.  Granted, it's a gray
area, since the problem with the connection is that it's not in an
open state....

-Kevin

On Wed, 18 Nov 2009, Kevin Grittner wrote:

> That all makes sense.  I think you have a copy/paste error in the
> exception message, though: "This statement has been closed."  It is
> the connection which has been closed here.

Doh! Fixed.

> Also, for software which tries to make sense of the SQLSTATE for
> automated handling, I'm inclined to think that something in Class Code
> '08' (Connection Exception) would be more useful than the Class Code
> '55' (Object Not in Prerequisite State) value.  Granted, it's a gray
> area, since the problem with the connection is that it's not in an
> open state....

I've changed it to 08003 CONNECTION_DOES_NOT_EXIST.  We're already using
it for this purpose (already closed) in the connection pooling code.

Thanks for following up on this.

Kris Jurka

Kris Jurka <books@ejurka.com> wrote:

> Thanks for following up on this.

Thank you for taking it all the way to what it should have been in the
first place.  I've got to try to overcome this tendency to submit the
minimum patch that will directly address a complaint.  I should be
willing to dive in and deal with the entire set of related issues, as
you do.

-Kevin