Connection.rollback() returns dirty connection to the pool.

[carl-mastrangelo]

Consider the following code:

    try (var conn= dataSource.getConnection()) {
      conn.setAutoCommit(false);
      conn.createStatement().execute(
          "INSERT INTO data (type, payload) VALUES ('a', '{}'::jsonb)");
      var savepoint = conn.setSavepoint();
      conn.rollback(savepoint);
    } catch (SQLException e) {
      throw new RuntimeException(e);
    }
    try (var conn= dataSource.getConnection()){
      conn.setAutoCommit(false);
      conn.commit();
    } catch (SQLException e) {
      throw new RuntimeException(e);
    }

I would expect the first connection closing to reset the state of the connection. However running this code (with HikariCP 5.0.1 and the Postgres adapter) still results in row being inserted. The reason is in the ProxyConnection.rollback(Savepoint), which sets isCommitStateDirty to false, even if there are pending changes in the transaction still.

[quaff]

https://github.com/brettwooldridge/HikariCP/blob/b40e6842080d2e7e5f8d56350aec20c49df0c9dd/src/main/java/com/zaxxer/hikari/pool/ProxyConnection.java#L392-L396

I think the isCommitStateDirty should be true.

[carl-mastrangelo]

I sent a PR. My gut reaction is that if setting a savepoint doesn't dirty the connection, releasing one shouldn't either. It's easier to reason about having the connection dirty on rollback, but I could also see just leaving the bit alone. Either way I'd be happy though.

[lfbayer]

If you set a savepoint on a connection that does not have uncommitted state, and then you rollback to that savepoint, shouldn't isCommitStateDirty be false? I imagine this is an unlikely flow, but does it have any unwanted side effects if isCommitStateDirty is true when there is nothing to do?

It seems like the dirty state should be set to the value of the dirty state as it was when the savepoint was set.

[carl-mastrangelo]

It seems like the dirty state should be set to the value of the dirty state as it was when the savepoint was set.

This would require a map from savepoint-to-dirty-bit which I think would be kind of an unnesecary cost to pay for the common case. I had considered it, but it optimizes for the relatively uncommon case of savepoint/rollback, which I'm okay pay the cost of connection re-establishment.