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
