JDBC improvements and convenience method suggestions

[Mark Rotteveel]

On 13-7-2018 09:50, Lukas Eder wrote:

> So, perhaps the overloads are just distracting from the actual feature 
> here...
> 
>     This default implementation assumes savepoint support, but savepoints
>     are an optional JDBC feature, we should not rely on optional features in
>     default implementations. I would suggest that a default implementation
>     should not use savepoints, but instead commit a current transaction (if
>     not in auto commit), and commit/rollback at the end.
> 
> 
> Sure, that could definitely be supported as well, and then there could 
> be flags to enable / disable one or the other behaviour. I would still 
> default to supporting nested transactions and risk throwing exceptions 
> once transactions actually *are* nested (if the driver doesn't support 
> savepoints), because nesting isn't the standard use-case.

I don't like the idea of making the default implementation more 
complicated then necessary, and I'm not convinced this needs to 
complexity of savepoints. And if that is a common use case, it should be 
explicit (see `inSavepoint(...)` c.s. suggested in my mail). To me 
`transaction(..)` suggests it is a full transaction.

> *Am Do., 12. Juli 2018 um 22:21 Uhr schrieb Mark Rotteveel 
> <mark at lawinegevaar.nl <mailto:mark at lawinegevaar.nl>>:*
> 
>     On 12-7-2018 17:43, Lukas Eder wrote:
>      > 3. Add Connection.executeXYZ() methods
> 
>     As Michael Rasmussen pointed out on Twitter in
>     https://twitter.com/jmichaelras/status/1017438847309926401, the
>     `executeQuery` variants will not work because the result set will be
>     closed by closing the statement. Instead I think that it should be
>     implemented as
> 
>           /**
>            * Create a statement and execute some SQL on it.
>            */
>           default ResultSet executeQuery(String sql) throws SQLException {
>               Statement s = createStatement();
>               try {
>                   s.closeOnCompletion()
>                   return s.executeQuery(sql);
>               } catch (SQLException e) {
>                   // Safeguard against lack of closeOnCompletion support
>                   try {
>                       s.close();
>                   } catch (SQLException suppressed) {
>                       e.addSuppressed(suppressed);
>                   }
>                   throw e;
>               }
>           }
> 
>     (+ similar for` executeQuery(String sql, Object... binds)`)
> 
> Yes, absolutely. This was an implementation oversight on my side. I had 
> previously prepared an implementation that would proxy the resulting 
> ResultSet and close the Statement upon ResultSet.close(). That 
> implementation somehow got lost.
> 
> Note, I had also suggested Connection.execute(String):boolean, which 
> does not make sense. Nothing can be done about this method returning 
> true. I will fix these two things.
> https://github.com/lukaseder/openjdk/commit/a05982f542b172121369ec131b93b028a6e1f17d

That could work, although this may still result in leaks if the result 
set is unwrapped and close is called on the unwrapped result set. Maybe 
combine it with use of `closeOnCompletion` as a second layer of defense?

Mark
-- 
Mark Rotteveel