AbstractJdbc4Connection.isValid() should not mark the transaction as started

[cowwoc]

Hikari connection pool invokes AbstractJdbc4Connection.isValid() before handing out a new connection. AbstractJdbc4Connection.isValid() ends up triggering QueryExecutorImpl.receiveRFQ:2270 which sets the transaction state to ProtocolConnection.TRANSACTION_OPEN. Subsequent calls to Connection.setTransactionIsolation() fail with Cannot change transaction isolation level in the middle of a transaction.

I don't think the specification meant to imply that invoking Connection.isValid() starts the transaction, thereby preventing users from invoking setTransactionIsolation(). I don't think it is the connection pool's responsibility to commit() or rollback() a transaction after invoking Connection.isValid(). I am not aware of any other database that behaves this way.

Expected behavior: transaction state should not be affected by call to AbstractJdbc4Connection.isValid()

[ringerc]

On 11/17/2014 04:40 PM, cowwoc wrote:

Expected behavior: transaction state should not be affected by call to
|AbstractJdbc4Connection.isValid()|

I agree, that's clearly a bug.

We should be sending an empty string query (""), or preferably just
sending a Sync protocol message. We certainly should not be doing
anything that takes a snapshot or affects transaction state.

I don't expect to have time to do anything about this any time soon
though, especially as I firmly believe that connection "validation" is
generally a symptom of bad application design.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

[cowwoc]

I don't expect to have time to do anything about this any time soon
though, especially as I firmly believe that connection "validation" is
generally a symptom of bad application design.

That flies in the face of all existing connection pool implementations. What do you expect them to do instead?

[ringerc]

On 11/17/2014 04:47 PM, cowwoc wrote:

That flies in the face of all existing connection pool implementations.
What do you expect them to do instead?

For connection pools it's reasonable, as it lets them fail-fast. A
connection pool is in a situation that applications don't otherwise
generally face, of having many idle connections. It'd be acceptable if
the pool handed the app a connection that was broken and the app said
"um, this one's broken, gimme another"... but you want to have that
replacement to have a good chance of actually working.

That's about the only generally reasonable use of Connection.isValid(), though. Using it to avoid retry logic is plain wrong, and using it instead of TCP keepalives is inefficient and unnecessary.

I routinely see people trying to write code along this pattern:

if (testIfConnValid(conn)) {
    codeThatAssumesConnIsValid(conn);
}

which is a hopeless race condition, as the connection can fail in between the test and actual use of it, or partway through using it.

The correct thing to do is generally do do everything in a retry loop with error-specific logic and a random backoff delay on retries, something closer to the pseudo-Java:

SQLException firstException;
boolean success = false;
int retries = 0;
while (!success && retries < MAX_RETRIES)
    try {
        doSomethingWithConn(conn);
    } except (SQLException ex) {
        if (retries == 0)
            firstException = ex;
        if (sqlStateErrorIsTransient(ex)) {
            // Deadlock abort, serialization failure, etc
            // that should work if we just retry the tx.
            retries++;
            logger.debug("Probably transient SQL error", ex);
            randomBackOffDelay();
            continue;
        } else if (sqlStateConnBroken(ex)) {
            // Might work with a reconnect...
            try {
                conn.close();
            } catch (SQLException ex) {
                logger.debug("When closing broken connection", ex);
            }
            retries++;
            logger.debug("Probably broken SQL connection", ex);
            randomBackOffDelay();
            conn = getNewConnection();
        } else {
            // Likely unrecoverable issue
            logger.error("Unexpected fatal SQL error", ex);
            break;
        }
    }
}
if (!success) {
    // Tell the user we couldn't do it and why based on the saved
    // first exception, number of retries, etc.
}

Yes, unfortunately that's much more complicated.

PgJDBC could make this easier for users by adding support for the JDBC4
exception class heirachy classes like SQLTransientException,
SQLRecoverableException, SQLNonTransientException, etc. Patches are of
course welcome ;-)

It's unfortunate that apps need to incorporate this kind of logic, and
it's something I'd like to see the JDBC spec tackle, perhaps by allowing
you to pass a Runnable (possibly to a Future-returning async function).
But for now, JDBC doesn't deal with it, so apps have to.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

[ringerc]

Anyway, the point is: I agree that the behaviour you describe is a bug. I don't expect to have time to work on it any time soon. If you want to submit a patch, which would be very welcome, I suggest that you:

• Add a new abstract method isValid to ProtocolConnection
• Add an implementation for v2.ProtocolConnectionImpl that sends an empty query-string "". You might have to make changes to the underlying executor to get PgJDBC to actually send it instead of simplifying it away, and to make sure PgJDBC handles the EmptyQueryResponse message type.
• Add an implementation for v3.ProtocolConnectionImpl that sends a Sync protocol message and waits for the ReadyForQuery response.

This may not be the best approach, as I haven't looked into it in detail, it's just where I'd start looking.

[brettwooldridge]

Try this workaround, set HikariCP isolateInternalQueries to true. You would also need to set a connectionTestQuery for this to work. It does seem that the pgjdbc-ng isValid() test should not leave a transaction in-progress.

[davecramer]

Currently the driver does a "select 1" I don't really see how it is
starting a transaction ? Is autocommit off ?

Dave Cramer

On 17 November 2014 04:22, Brett Wooldridge notifications@github.com
wrote:

Try this workaround, set HikariCP isolateInternalQueries to true.

—
Reply to this email directly or view it on GitHub
#214 (comment).

[ringerc]

On 11/17/2014 07:13 PM, Dave Cramer wrote:

Currently the driver does a "select 1" I don't really see how it is
starting a transaction ? Is autocommit off ?

Autocommit shouldn't matter, I think that's the point. It should run
within an open transaction if there is one, or as a standalone statement
if there's no open transaction.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

[cowwoc]

@brettwooldridge According to brettwooldridge/HikariCP#196 setting isolateInternalQueries to true will be ignored (because isUseJdbc4Validation is true).

@davecramer Yes, autocommit is off but as @ringerc stated it shouldn't matter.

[cowwoc]

@brettwooldridge and @ringerc I've updated http://stackoverflow.com/a/17960047/14731 based on your recommendations. Please let me know if I've missed anything.

[ringerc]

On 11/18/2014 01:49 AM, cowwoc wrote:

@davecramer https://github.com/davecramer Yes, autocommit is off but
as @ringerc https://github.com/ringerc stated it shouldn't matter.

Yes, or more specifically, I think that the current behaviour described,
where isValid() affects the transaction state, is incorrect as it stands.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

[polobo]

Actually, by definition isValid, if executed while a transaction is in progress, will affect its state since the query that is sent is executed in the context of said transaction just like any other user query.

https://docs.oracle.com/javase/6/docs/api/java/sql/Connection.html#isValid(int)

The issue here is that the pool must return a known good connection but must first ensure that when it does so that the current session is not in transaction. Ideally isValid would be aware enough of its state to know whether it needs to issue a commit after sending the heartbeat query. It needs to read the session and confirm in or out of transaction before sending the heartbeat query and issue commit as appropriate once the query result returns.

The locking involved with the issuance of "select 1" versus some other query warrants discussion - even if we'd frown upon mid-transaction calls to isValid - but falls outside the scope of this report and the failure of isValid to leave the connection in the same transaction state it started with.

[ringerc]

On 11/18/2014 01:00 PM, David Johnston wrote:

Actually, by definition isValid, if executed while a transaction is in
progress, will affect its state since the query that is sent is executed
in the context of said transaction just like any other user query.

https://docs.oracle.com/javase/6/docs/api/java/sql/Connection.html#isValid(int)
https://docs.oracle.com/javase/6/docs/api/java/sql/Connection.html#isValid%28int%29

Yes, but what about autocommit state?

If autocommit is off, isValid() shouldn't be opening a new transaction,
pinning the transaction isolation level and read/write mode, setting the
transaction timestamp, and establishing a server-side snapshot (if
SERIALIZABLE).

It must round-trip to the server to ensure it's alive, but it shouldn't
be creating noise in the server logs or doing things that might affect
the state of an existing user transaction or change the meaning of
subsequent user commands.

That's why I think that for PostgreSQL we should be sending a Sync
message, or the empty query string.

It'd be good to see what the spec says and what other drivers do in this
situation.

The locking involved with the issuance of "select 1" versus some other
query warrants discussion - even if we'd frown upon mid-transaction
calls to isValid - but falls outside the scope of this report and the
failure of isValid to leave the connection in the same transaction state
it started with.

It's actually directly relevant, as only some commands will acquire a
transaction snapshot, etc.

We can send a Sync message or an empty string without affecting the
transaction state in any way. That's not true for "SELECT 1".

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

[brettwooldridge]

@ringerc In my opinion, the isValid() call should do the minimum required to determine that the connection is "live". If this can be done without running a query that would initiate a transaction that would be great. From my experience, MySQL does not initiate a transaction, but rather sends (literally) the String /* ping */ to the server.

But HikariCP does have a "backdoor" for drivers that do initiate transactions from isValid() or test queries, and that is to define an explicit query (which disables the isValid() check), and set isolateInternalQueries to true which will perform a rollback() provided that auto-commit is false.

[davecramer]

Seems simple enough then, we can change the current code to do select ""
until someone has time to send sync

Dave Cramer

On 18 November 2014 00:35, Brett Wooldridge notifications@github.com
wrote:

@ringerc https://github.com/ringerc In my opinion, the isValid() call
should do the minimum required to determine that the connection is "live".
If this can be done without running a query that would initiate a
transaction that would be great. From my experience, MySQL does not
initiate a transaction, but rather sends (literally) the String /* ping */
to the server.

But HikariCP does have a "backdoor" for drivers that do initiate
transactions from isValid() or test queries, and that is to define an
explicit query (which disables the inValid() check), and set
isolateInternalQueries to true which will perform a rollback() provided
that auto-commit is false.

—
Reply to this email directly or view it on GitHub
#214 (comment).

[ringerc]

On 11/18/2014 06:52 PM, Dave Cramer wrote:

Seems simple enough then, we can change the current code to do select ""
until someone has time to send sync

select ''

would have the same issue.

It'd have to send the empty string as a query. No SELECT.

IIRC PgJDBC might not actually send such queries to the server at the
moment, it might just determine client-side that they're empty; I
haven't looked in detail though.

Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services