On Thu, 2 Aug 2007, Oliver Jowett wrote:
> I didn't dig into the code too closely but it looks like you are using the
> statement object directly with no wrapper. Doesn't this run the risk that you
> will resurrect a previously-closed statement? Normal statement objects have a
> one-way lifecycle, once they are closed they cannot be resurrected, if app
> clients have a reference to the real statement then potentially they'll see
> different behaviour when the statement starts getting reused. That smells
> dangerous; not because any sane application will rely on it, but because it
> will be a source of very hard to find bugs. (e.g. it's fairly common and
> harmless to close an already-closed statement.. but that's suddenly
> disastrous if the statement has actually been pooled & reused in the
> meantime)
>
This is the fundamental objection. Calling close multiple times is
perfectly legal and is not supported by this implementation. I have some
additional notes based on my reading of the code that are rather secondary
to the above:
1) Why does PStmtKey default holdability to HOLD_CURSORS_OVER_COMMIT when
the driver defaults to close at commit?
2) What is the point of getNumActive/Idle in AbstractJdbc3StatementPool?
3) As you note it doesn't build with JDK1.6, but it also doesn't build
with JDK1.2 or 1.3. While statement pooling is a JDBC3 feature the
other driver versions must still build.
4) The test suite you've provided fails for me with:
junit.framework.AssertionFailedError
at
org.postgresql.test.jdbc3.Jdbc3CacheableStatementTest.testStatementCacheBehaviour(Jdbc3CacheableStatementTest.java:104)
5) Defaulting the cache size to unlimited seems unwise.
6) You can't add tests to jdbc2/optional that require JDBC3 functionality.
7) What's the deal with caching CallableStatements? It looks half
finished and should either be removed or implemented.
8) Jdbc3CacheablePreparedStatement references Jdbc3ResultSet, but doesn't
it need to refer to Jdbc3gResultSet if building with JDK1.5? I don't
understand how this compiles, but it does.
Kris Jurka
