Re: binary patch problems - Mailing list pgsql-jdbc

On 09/17/2011 02:14 AM, Oliver Jowett wrote:
> On 17 September 2011 01:20, Mikko Tiihonen
> <mikko.tiihonen@nitorcreations.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.

Thank you for the thorough analysis. Now we need to decide what to do
with the ForceBinaryTransfers testing/debugging aid.

It was originally added to ensure that while running the unit tests
as much as possible of the code paths would exercise the binary transfer
code. Normally binary transfers would only be activated after few
round-trips to the backend with the same prepared statement - which
basically never happens in unit tests.

We have at least three options:
1) drop the ForceBinaryTransfers
    - it has helpped debug the binary transfer to a working state
    - it is no longer that useful
2) leave it as is
    - just add the hack to the one failing test to make it not fail
    - it is still useful to run the test suite to verify functionality
      stays correct with binary transfers
3) fix it
    - less worries in the future and if some end user finds the
      undocumented feature they won't get bitten by it

-Mikko