Deepa Remesh wrote: > On 2/13/06, Daniel John Debrunner (JIRA) <[email protected]> wrote: > >> [ >> http://issues.apache.org/jira/browse/DERBY-210?page=comments#action_12366232 >> ] >> >>Daniel John Debrunner commented on DERBY-210: >>--------------------------------------------- >> >>The use of close() for close and reset is confusing (I know it's existing >>code). >> >>Your addition is now the only code in close() that actually generates new >>objects. This code >>will be called even when the statement is being closed in order that it no >>longer be used. >>This might have a performance impact, I don't know how often in a well >>behaved application >>this internal close method would be called. Once per statement execution or >>creation? >> >>I'm unclear on what this patch ('derby-210-patch3.diff') is addrressing. The >>comments for >>item 10) above don't actually help me much. Was the old code successfully >>re-using the old >>DRDAResultSet() but now for some reason we force it to use a new one every >>time?
Thanks for the explanation. > The old code was successfully re-using DRDAResultSet. This patch will > be required after my changes to the finalization code in client > driver. With these changes, a CLSQRY is not sent for each result set > to network server during finalization. The cleanup of result sets is > expected to happen when the DRDAStatement is reset. But this was not > happening and I was getting a failure in derbynet/prepStmt.java when I > was running tests with the full set of changes. One solution to this > was to create a new DRDAResultSet, same as what would be done for a > new statement. I added this and a comment to say "close() method is > also called before re-using a statement in the method > Database.newDRDAStatement. It has to ensure that the statement state > is restored. ". I hope this explains the reason for this patch. 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. > 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. Thanks for all the effort you are putting into 210. Dan.
