Skip site navigation (1) Skip section navigation (2)

Re: statement caching patch from Laszlo Hornyak for review

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 (view raw or flat)
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


In response to

Responses

pgsql-jdbc by date

Next:From: Miroslav ŠulcDate: 2007-08-03 17:29:29
Subject: Failing test on 8.2-506
Previous:From: Dennis ThrysøeDate: 2007-08-03 06:54:30
Subject: Re: LargeObject API

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group