Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: <derby-dev@db.apache.org>
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <43E65131.30206@amberpoint.com>
Date: Sun, 05 Feb 2006 11:25:37 -0800
From: Bryan Pendleton <bpendleton@amberpoint.com>
User-Agent: Thunderbird 1.5 (Windows/20051201)
MIME-Version: 1.0
To: derby-dev@db.apache.org
Subject: DERBY-821 comments
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Knut Anders,

Thank you again for doing such complete work on this patch. The change
description is very helpful, and the comments in the code are quite good, too.

Your patch applies cleanly for me; derbyall passes cleanly with your patch
applied; I tried a variety of simple tests and stepped through the code on
both the client-side and on the server-side, and it all worked as I expected
(and as you described). I didn't see anything that I felt needed to be changed.

I do have a number of questions, though, which I was hoping you could take
the time to clear up for me. A lot of my questions are because I'm still
trying to learn how the client-side code works, and this was a good excuse
to explore a bunch of code that I hadn't yet had the time to learn. So
thanks for giving me that good excuse!

bryan

- Do we have any contacts on the JCC team who could help us understand
their interpretation of QRYCLSIMP? In particular, it's kind of gross that
DRDAStatement.isRSCloseImplicit() has to have a special case handling of JCC,
and it would be nice to know why the JCC driver doesn't behave as we
expect it should.

- When I read about QRYCLSIMP in volume 3 of the DRDA spec, I was rather
surprised to see that it was actually not a boolean value, but an enumerated
type. After re-reading that section multiple times, I am still confused
about the difference between the values "YES" and "SERVER_CHOICE". Here
are some of my confusions:
    - In the SERVER_CHOICE case, how would the client know what the server's
      choice actually was? It doesn't seem like the server sends any
      information back on the OPNQRYRM to let the client know what it decided.
    - In the YES case, it seems like the server must *always* implicitly close
      the query, no matter what the cursor type and attributes are. But that
      doesn't make sense; surely the server is NOT supposed to
      implicitly close the query in the case of a scrollable cursor?
      I see that the spec says "IGNORABLE If query is scrollable", but then
      what is the difference between YES and SERVER_CHOICE? I guess that I
      find this whole section rather confusing.

- Given what I read about QRYCLSIMP in the DRDA spec, it seems to me that
it would be more accurate for the net client to be sending QRYCLSIMP value
SERVER_CHOICE, rather than value YES, which is what you have coded. That is,
your client implementation expects the server to determine whether or not
to implicitly close the query based on the cursor type, which is the
description that goes with SERVER_CHOICE.

- It also seems to me that the method DRDAResultSet.setOPNQRYOptions is
perhaps inaccurate. Currently, that method seems to say:
   - if the client sent QRYCLSIMP=YES, then perform implicit close
   - if the client sent QRYCLSIMP=NO, then don't perform implicit close
   - if the client sent QRYCLSIMP=SERVER_CHOICE, then don't perform implicit close.
That last line seems wrong to me. It seems to me that if the client sent
SERVER_CHOICE, the server should perform implicit close, but as I read the
code we currently will not perform implicit close. Of course, this may be
somewhat a moot point, because maybe we don't know of any clients which
actually send SERVER_CHOICE, but it still worries me.

- I don't understand how cursor holdability is supposed to come into this
feature. The DRDA spec seems to say that we are not supposed to implicitly
close a query if was declared with the HOLD attribute, but I didn't see
any places in your implementation where you incorporated the cursor
holdability into the implicit close logic. Do you understand what the DRDA
spec is trying to say with respect to implicit close and holdability, and
can you help me understand how it should be translated into actual code in
the implementation?

- When I first saw these two lines in SqlCode.java, I did a double-take:

    /** SQL code for SQL state 02000 (end of data). */
    public final static SqlCode END_OF_DATA = new SqlCode(100);

I thought that the '100' was a typo, and it was supposed to be '2000'. Later,
I realized that I had become confused between SQLState and SQLCode, and
that testing for Code=100 is how the client implements testing for
State=2000. Perhaps there is some what to make this a bit more obvious
to the next reader.

- I don't understand why we don't do implicit close for result sets
returned as output parameters of EXCSQLSTT procedures. Can we not implicitly
close these result sets for some reason? It worries me that on the server
side, DRDAStatement.isRSCloseImplicit() does not seem to have any code for
handling the result sets differently, so it seems like maybe the Server *is*
implicitly closing EXCSQLSTT result sets, even though the client thinks it
is not implicitly closing them. Can you expand on this section of your
patch a little more to make this behavior more clear? Do you think that this
is a place where having an extra regression test in the test suite would help?

- I didn't understand why you removed the two SingletonCursor methods in
ResultSet.java. This didn't seem related to the other changes in your patch.
Can you expand on why you removed these methods?

- When I first saw that you added a Session pointer to the Database object
in the server code, I was worried, because I thought that maybe Database
objects were server-wide objects and might be shared by multiple sessions.
I later persuaded myself that in fact the Database object is a per-session
object, and so there is no such sharing violation. However, I'm still
somewhat troubled by this change. It seems like the original designers went
to some trouble to set up these objects so that the Session points to the
Database, not vice versa, and so I worry about the implications of storing
a Session pointer in the Database object. From what I can tell, the only
reason that you need to have the Session pointer in the Database object is
so that you can perform the special check for the JCC handling in
DRDAStatement.isRSCloseImplicit(), is that right? If so, perhaps it's worth
looking at this a little while longer, to see if there is a way to handle
this without having to add the session pointer to the database object; maybe
you could instead arrange to pass the session pointer as an argument when
you call isRSCloseImplict?

- Frankly, all the business about stepNext() and preClose() and
scanDataBufferForEndOfData() seemed rather awkward to me. It seemed like a
lot of complexity to be adding to the code, and resulted in some constructions
such as the 'allowServerFetch' argument to calculateColumnOffsetsForRow()
which certainly complicated an already quite complex bit of code. If I'm
understanding all of this correctly, the case you are handling here is the
following:
   - the client has fetched some, but not all, of the rows in the result set
   - the server has already fetched and returned all the rows in the result
     set, so they are sitting in a network buffer on the client side
   - the client calls ResultSet.close() before it finishes reading those rows
In this case, the scanDataBufferForEndOfData() routine will artificially
"read" through all these buffered rows in order to see if it can avoid
sending the CLSQRY to the server. I think that your implementation is correct;
I had no trouble reproducing this situation in a test case and stepping through
the code and it all seemed like it was working properly. My concern is simply
I just wonder how important this particular optimization is, compared to
the overall implicit close optimization. It was easy for me to make it occur
in a test case; I just suspect it doesn't arise very often in real life. And
I also wonder how much it saves you: you save the CLSQRY, but you add the
expense of scanning the client-side data buffer. So it's somewhat of a
tradeoff, isn't it?

- I spent a bit of time thinking about some additional test cases and new
situations that might arise. I don't know how important these are, but if
you had some time, you might try writing a few other tests to add to the test
suite. These are the ones that occurred to me:
  - closing the client side result set before fetching all the rows, both in
    the case where the implicit close has already occurred on the server side,
    and in the case where it has not yet occurred on the server side.
  - situations in which the client fetches all the rows normally, causing an
    implicit close on the server side, but some sort of an error occurs
    during the implicit close on the server side.
  - situations in which the server performs an implicit close, but the client
    sends a CLSQRY anyway. I guess this would indicate a client bug, but it
    would be interesting to know how the server deals with it.