Re: binary patch problems

On 09/19/2011 01:03 PM, Dave Cramer wrote:
> Hi Mikko,
>
> My suggestion would be to fix it.

Finally had some time to look into this.
Closing the result set of the describe query fixes the test failure for me.

-Mikko

--- org/postgresql/jdbc2/AbstractJdbc2Statement.java.orig 2011-09-20 16:55:18.130947602 +0300
+++ org/postgresql/jdbc2/AbstractJdbc2Statement.java 2011-09-20 16:56:02.310694214 +0300
@@ -541,6 +541,8 @@
int flags2 = flags | QueryExecutor.QUERY_DESCRIBE_ONLY;
StatementResultHandler handler2 = new StatementResultHandler();
connection.getQueryExecutor().execute(queryToExecute, queryParameters, handler2, 0, 0, flags2);
+ ResultWrapper result2 = handler2.getResults();
+ result2.getResultSet().close();
}

StatementResultHandler handler = new StatementResultHandler();

> On Fri, Sep 16, 2011 at 7:14 PM, Oliver Jowett<oliver(at)opencloud(dot)com> wrote:
>> On 17 September 2011 01:20, Mikko Tiihonen
>> <mikko(dot)tiihonen(at)nitorcreations(dot)com> wrote:
>>
>>> If I debugged it correctly it looks like the query statement is closed by
>>> finalizer during the test which in turn closes the relevant portals.
>>> If I add stmt.close() to the end of the test there are no failures as it
>>> keeps the GC from killing of the statement during the test.
>>>
>>> But why I only seem to get it when running with binary transfer patches I do
>>> not know. So there must be something in the patches that I cannot spot.
>>
>> (I am halfway through my first coffee of the day. YMMV)
>>
>> The test code holds a reference to the returned ResultSet, but doesn't
>> keep a reference to the Statement itself after calling executeQuery().
>> Normally this isn't a problem because there is a strong ref from a
>> ResultSet to its owning Statement, so while you are using a particular
>> ResultSet the creating statement remains reachable.
>>
>> The problem seems to be introduced in the patch to executeQuery if
>> (ForceBinaryTransfers). This delegates to a new PreparedStatement
>> ("inner PreparedStatement") and splices its resultset ("inner
>> resultset") back into the surrounding Statement ("outer Statement")
>> and returns that.
>>
>> However the inner ResultSet has the wrong parent Statement for that
>> context (ResultSet.getStatement() will return the "inner
>> PreparedStatement" not the "outer Statement") - which is a separate
>> problem, but also causes the GC problems as well, as now there's no
>> strong ref from the returned ResultSet to the Statement that the
>> client created it via.
>>
>> So in the test code we end up with:
>> (a) a strong ref to the inner ResultSet, directly; and
>> (b) a strong ref to the inner PreparedStatement, via the statement
>> reference of the inner ResultSet.
>>
>> .. but no strong ref to the outer Statement. So the outer Statement
>> becomes garbage right after the call to executeQuery(), and if you are
>> unlucky with GC timing, it can be finalized and closed under you,
>> which then closes the inner ResultSet.
>>
>> Adding a call to stmt.close() means that the outer Statement still has
>> a strong reference directly from test code up until that point, so it
>> doesn't get finalized early.
>>
>> At a glance, executeQuery() is also broken for queries that return
>> multiple resultsets if ForceBinaryTransfers is enabled - you only get
>> the first resultset back.
>>
>> Oliver
>>