db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Deepa Remesh <drem...@gmail.com>
Subject Re: [jira] Commented: (DERBY-210) Network Server will leak prepared statements if not explicitly closed by the user until the connection is closed
Date Wed, 15 Feb 2006 21:31:45 GMT
Thanks Dan for looking at this change. By doing the change I submitted
in the patch, I could confirm what/where the problem is but it has to
be narrowed down further. After seeing your comment on patch3, I have
been trying to do this and also see if there is a better way to solve
this. Please see my answers for your questions inline.

> I guess it's still not clear to me. It's also not clear from the
> comments in the code, if someone looked in six months at this code and
> compared it to the previous version, they would not see any reason as to
> why the code stopped re-using an object.
>
> I'm pushing for an explaination because I think fixing 210 is important
> for Derby but it's an area we need to get correct. My gut feeling is
> telling me that the fix is of the form "doing this make the test pass
> but I don't really know why", but it could be because I don't know the
> code, I'm missing context/implications that you see because you do know
> the code. I feel if the coder can not explain the fix clearly, then
> there's a good chance the fix is incorrect, leading to bugs in the future.
>

Your gut feeling is right. I submitted this patch to get feedback
about my change and to see if I was on the right track. I knew the
cause of the failure I got in derbynet/prepStmt.java was because
currentDrdaRs was not getting reset when statements are not explicitly
closed. And I could verify that this was indeed the cause by checking
the problem goes away on recreating the result set when statement is
re-used. I tried to see this does not cause any performance impact. I
have tried to explain my reasoning for this using your example below.
As I found there was no negative performance impact, I submitted this
patch. Also, it is the same method which is getting called when
closing/re-using the DRDAStatement. So I presumed it is okay to do
this.

On looking some more, I could narrow down the problem further and
think the problem is in DRDAResultSet.close(). In this close() method,
splitQRYDATA variable does not get reset. I think this (combined with
my other changes for derby-210) is what is causing the intermittent
failure in derbynet/prepStmt.java. I ran prepStmt test with (change to
reset splitQRYDATA in DRDAResultSet.close) +
(all_my_changes_for_DERBY-210) and the test passes. But the thing is,
the test is also passing now without this change. (The failure was
intermittent in the first place.) So I am again left without a repro
to confirm this.

As I mentioned before, prepStmt test was failing in the test added for
Jira125 where it calls rs.next(). Bryan, since you have worked on
DERBY-125 and DERBY-614 and also familiar with my changes for
DERBY-210, can you please take a look to confirm my finding about
splitQRYDATA? I think we should be resetting this in the close()
method irrespective of my changes for DERBY-210.

It would have been easy to debug this if I could get client and server
traces for this. Unfortunately, I have not been able to simulate a
failure when trace is on. If I can collect traces, I will post it.

> > As I mentioned before, I was thinking if it was okay to add a "new"
> > into the close method. I did not see any performance impact since
> > close() gets called when re-using statements or when the session is
> > closed. During a session, DRDAStatement.close() method gets called
> > only when the statement is re-used. When session is closed,
> > DRDAStatement.close() gets called for each statement in the statement
> > table and the statement also gets removed from the hash table. From
> > what I understood, I think there is no performance impact because of
> > this change. Please correct me if I am wrong here.
>
> Is it possible to re-write this to be clear if you are referring to the
> JDBC statement object the application is using or the DRDAStatement.
> It's another unfortunate situation where the code overloads the term
> statement. If the relationship is written up elsewhere, please point me
> to it. For example, with this piece of JDBC code, at what points is the
> DRDAstatement closed, re-used etc.
>
>  Statement s = conn.createStatement();
>
>   ResultSet rs = s.execute(Q1);
>   .. process rs
>   rs.close();
>
>   rs = s.execute(Q1);
>   .. process rs
>   rs.close();
>
>   rs = s.execute(Q2);
>   .. process rs
>   rs.close();
>
>   s.close();
>
> I'm interested in knowing when (and hence how often) with this change
> Derby will be creating a new object instead of re-using one, and with
> the change as it is, how often it is creating a new object just for it
> to be thrown away.

In your example, I can think of these situations:

1. If nothing further is done in the application and client
disconnects its session, DRDAStatement#close() method gets called for
all statements stored in the statement hashtable when the session is
closed. In this case, a new DRDAResultSet object gets created but get
freed immediately because there won't be any references to it.

2. If no other statements are created in the client application,
DRDAStatement#close method will not be visited in the network server.
(till client disconnect/server shutdown)

3. If the application creates another statement after closing the
first one, something like:

s.close();
...
Statement s2 = conn.createStatement();

In this case, client driver will re-use the freed section number from
's' for 's2'. In network server, it will find that there is a
statement in the statement table with same section number  and it will
get this statement, call a close() on it and re-use the statement.
When DRDAStatement.close() is called, a new DRDAResultSet object will
be created and assigned to currentDrdaRs. The previous object
reference in currentDrdaRs is freed. Again, there won't be any extra
objects in network server.

Does this reasoning sound correct?

>
> Thanks for all the effort you are putting into 210.

Thanks to all for patiently reading through my patches/comments and
providing guidance.

Thanks,
Deepa

Mime
View raw message