From: | Dave Cramer <pg(at)fastcrypt(dot)com> |
---|---|
To: | Kris Jurka <books(at)ejurka(dot)com> |
Cc: | Oliver Jowett <oliver(at)opencloud(dot)com>, List <pgsql-jdbc(at)postgresql(dot)org>, Laszlo Hornyak <laszlo(dot)hornyak(at)gmail(dot)com> |
Subject: | Re: statement caching patch from Laszlo Hornyak for review |
Date: | 2007-08-03 11:02:48 |
Message-ID: | 33E43614-DF0E-4913-92DB-2E8EC4B1E156@fastcrypt.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-jdbc |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Miroslav Šulc | 2007-08-03 17:29:29 | Failing test on 8.2-506 |
Previous Message | Dennis Thrysøe | 2007-08-03 06:54:30 | Re: LargeObject API |