hi Jeremy,
I'm looking at your patch, and I will take a look at the connection state interceptor. most SQL exceptions, as you say, should percolate through when it makes sense.

I wont apply the patch as it is, as I don't understand why most of the methods just change the signature of the method by adding "throws SQLException".

for example ConnectionPool.abandon doesn't really need to throw a SQLException

best
Filip

Hello,

This initial use-case I was needing was for SQLExceptions originating in ConnectionState.reset() to propagate out of ConnectionPool.getConnection().

The rest of the changes cascaded from this:  eg: ConnectionPool.abandon() -> ConnectionPool.release() -> PooledConnection.setHandler() -> JdbcInterceptor.reset().

I suspect there will be more and that most APIs will possibly throw SQLException...

Thanks.

I see, exceptions can't simply be propagated like that, since that would cancel out events/code that needs to run.

    protected void finalize(PooledConnection con) {
        JdbcInterceptor handler = con.getHandler();
        while (handler!=null) {
            handler.reset(null, null);
            handler=handler.getNext();
        }
    }

If a reset() throws an exception, the rest of the interceptors wouldn't be notified here, and the exception would not mean very much.

So, probably better to address case by case, and see what needs to be done

best
Filip

(In reply to comment #3)
>     protected void finalize(PooledConnection con) {
>         JdbcInterceptor handler = con.getHandler();
>         while (handler!=null) {
>             handler.reset(null, null);
>             handler=handler.getNext();
>         }
>     }
> 
> If a reset() throws an exception, the rest of the interceptors wouldn't be
> notified here, and the exception would not mean very much.

Currently, if a connection pool is configured for a database that does not exist, the first place a SQLException occurs is reset() (with the following call stack for mysql):

Caused by: com.mysql.jdbc.exceptions.jdbc4.MySQLSyntaxErrorException: Unknown database 'db_name'
	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:39)
	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:27)
	at java.lang.reflect.Constructor.newInstance(Constructor.java:513)
	at com.mysql.jdbc.Util.handleNewInstance(Util.java:407)
	at com.mysql.jdbc.Util.getInstance(Util.java:382)
	at com.mysql.jdbc.SQLError.createSQLException(SQLError.java:1052)
	at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3593)
	at com.mysql.jdbc.MysqlIO.checkErrorPacket(MysqlIO.java:3525)
	at com.mysql.jdbc.MysqlIO.sendCommand(MysqlIO.java:1986)
	at com.mysql.jdbc.MysqlIO.sqlQueryDirect(MysqlIO.java:2140)
	at com.mysql.jdbc.ConnectionImpl.execSQL(ConnectionImpl.java:2620)
	at com.mysql.jdbc.ConnectionImpl.setCatalog(ConnectionImpl.java:5116)
	at org.apache.tomcat.jdbc.pool.interceptor.ConnectionState.reset(ConnectionState.java:95)
	at org.apache.tomcat.jdbc.pool.ConnectionPool.setupConnection(ConnectionPool.java:280)
	at org.apache.tomcat.jdbc.pool.ConnectionPool.getConnection(ConnectionPool.java:169)
	at org.apache.tomcat.jdbc.pool.DataSourceProxy.getConnection(DataSourceProxy.java:111)

If reset() swallows and logs this and we don't propagate it, then the upper layer has no way of acting on this (or controlling the log output).

We can try finishing executing the interceptor stack, ie:

protected void finalize(PooledConnection con) {
    JdbcInterceptor handler = con.getHandler();
    Exception exception = null;
    while (handler!=null) {
        try {
            handler.reset(null, null);
        } catch (SQLException e) {
            exception = e;
        }
        handler=handler.getNext();
    }
    if (exception != null) {
        throw exception
    }
}

but, if more exceptions are thrown from additional interceptors, what exception should ultimately be thrown from finalize()?

One solution is to NOT continue processing the interceptor stack.  If interceptors need to be notified, we could use another event.

Thoughts?

Sorry, the example code above should be for ConnectionPool.setupConnection().  The problem is the same though; do we continue invoking the interceptors on an exception or not.

What I propose, is to fix the actual bug, of ConnectionState swallowing the exception on pool startup.
And we fix this bug, without introducing new bugs by changing too many signatures.

Agreed, the actual bug is ConnectionState swallowing the exception on pool startup, however I suspect that fixing it properly will require some of the signatures to change.

In my opinion, SQLException's needs to propagate all the way up to ConnectionPool.getConnection().  SQLException is a checked exception so when we propagate it from JdbcInterceptor, it will affect the callers signature unless that caller swallows it (not what we want) or wraps it in a RuntimeException (a bit clumsy).

Thanks for your time and discussion, it's very appreciated.

I resolved this bug in SlowQueryReport，using the second proposal by Felix Schumacher -- "extract the information to sort on from the QueryStats and sort on that information".

1, add a constructor in the inner class QueryStats for QueryStats clone
        /**
         * Create a new QueryStats object based on another QueryStats object.
         * For resolving Bug 58489, https://bz.apache.org/bugzilla/show_bug.cgi?id=58489
         * @param qs
         */
        QueryStats(QueryStats qs){
            this.query = qs.query;
            this.nrOfInvocations = qs.nrOfInvocations;
            this.maxInvocationTime = qs.maxInvocationTime;
            this.maxInvocationDate = qs.maxInvocationDate;
            this.minInvocationTime = qs.minInvocationTime;
            this.minInvocationDate = qs.minInvocationDate;
            this.totalInvocationTime = qs.totalInvocationTime;
            this.failures = qs.failures;
            this.prepareCount = qs.prepareCount;
            this.prepareTime = qs.prepareTime;
            this.lastInvocation = qs.lastInvocation;
        }

2, modify method removeOldest
    protected void removeOldest(ConcurrentHashMap<String,QueryStats> queries) {
        ArrayList<QueryStats> list = new ArrayList<>(queries.values());
        //For resolving Bug 58489, https://bz.apache.org/bugzilla/show_bug.cgi?id=58489
        //make a list of cloned QueryStats objects
        for(int i=0; i<list.size(); i++){
            list.set(i, new QueryStats(list.get(i)));
        }
        //
        Collections.sort(list, queryStatsComparator);
        int removeIndex = 0;
        while (queries.size() > maxQueries) {
            String sql = list.get(removeIndex).getQuery();
            queries.remove(sql);
            if (log.isDebugEnabled()) log.debug("Removing slow query, capacity reached:"+sql);
            removeIndex++;
        }
    }