Re: statement caching patch from Laszlo Hornyak for review

Kris, Oliver,

Laszlo is working on all of your recommendations, thanks for taking
the time to review this.

DAve
On 2-Aug-07, at 1:02 PM, Kris Jurka wrote:

>
>
> 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.testStatementCac
> heBehaviour(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
>
> ---------------------------(end of
> broadcast)---------------------------
> TIP 9: In versions below 8.0, the planner will ignore your desire to
>       choose an index scan if your joining column's datatypes do not
>       match