Too pessimistic handling of SQLFeatureNotSupportedException for Postgres binary fields

[acehead]

Environment

HikariCP version: 3.4.1
JDK version     : 1.8.0_222-b10
Database        : PostgreSQL
Driver version  : 42.2.8

Hikari closes connection on SQLFeatureNotSupportedException which leads to problems handling scenarios when exact driver implementation is not known so client code can execute potentially not implemented operations.

Specifically, this happens when using Hikari with Postgres as a connection pool for ActiveJDBC layer. ActiveJDBC tries to implement both Oracle and Postgres ways of storing binary data (https://github.com/javalite/javalite/blob/master/activejdbc/src/main/java/org/javalite/activejdbc/DB.java#L690) and handles SQLException itself. However subsequent use of the setObject() fails as the connection is already closed by Hikari.

This might seem to look like ActiveJDBC issue although I cannot propose any way for them to handle this situation in a driver-agnostic way. Hikari approach seems to be rather unexpected and was introduced in #1003

I haven't yet filed an issue for ActiveJDBC, it would be really appreciated if this would be fixed either in Hikari, or there could be a good way to implement it in client code like ActiveJDBC. I would like to avoid hackish solutions like putting custom HikariProxyConnection on the classpath.

[westse]

Created PR (#1509) to help address this generally.

Regarding SQLFeatureNotSupportedException (sql state 0A000) some engines (Vertica) will partially support some features (like altering column types) and eviction is not ideal. Instead the caller should be able to handle the error and reuse the connection. PR #1509 addresses this generally by making configurable all exceptions/states/codes that would result in eviction.

[westse]

For what it's worth, until this is fixed (via #1509 or otherwise) we are working around this issue with the following:

    private final HikariDataSource dataSource; // initialized elsewhere
    private static volatile boolean errorStateHackApplied = false;

    /**
     * Hikari ProxyConnection tries to handle error states returned from the database
     * which does not handle '0A000' appropriately. It was introduced to fix:
     * https://github.com/brettwooldridge/HikariCP/issues/1003
     * This change causes the connection to automatically get closed by Hikari when the database returns
     * a state 0A000, which is not correct in all cases.
     *
     * A subsequent ticket was opened (https://github.com/brettwooldridge/HikariCP/issues/1489) to challenge
     * the handling of '0A000' as a connection failure.
     *
     * For now, we will just remove the inappropriate error state handling via reflection
     * ERROR_STATES.add("0A000"); // FEATURE UNSUPPORTED
     */
    private void applyErrorStateHack() {
        if (!errorStateHackApplied) {
            try (Connection conn = dataSource.getConnection()) {
                if (conn instanceof ProxyConnection) {
                    Field field = ProxyConnection.class.getDeclaredField("ERROR_STATES");
                    field.setAccessible(true);
                    Set<String> errorStates = (Set<String>) field.get(conn);
                    errorStates.remove("0A000");
                    errorStateHackApplied = true;
                }
            } catch (SQLException sqle) {
                LOGGER.warn("Unable to apply error state hack", sqle);
            } catch (Exception e) {
                LOGGER.error("Unable to apply error state hack", e);
                System.exit(1);
            }
        }
    }

[brettwooldridge]

@acehead @westse I have implemented this in a slightly different way than pull request #1509. This change keeps the configuration surface area to a minimum, as well as allowing purely text-based (e.g. property-based) configuration.

You can see an example usage here and here.

This will likely be released in several days along with several other fixes and after the completion of documentation.

[johnou]

@brettwooldridge did you also see the feedback from this issue? #1388 (comment)

[eicki]

I'm fine with this solution. Nevertheless I still think it is wrong in the majority of cases to consider SQLTimeoutException an eviction reason as implemented due to #1308. I think handling SQLTimeoutException is something for the application to handle.

Maybe you should consider adding a default SQLExceptionOverride that returns DO_NOT_EVICT for SQLTimeoutException.