db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Knut Anders Hatlen <Knut.Hat...@Sun.COM>
Subject Re: DERBY-821 comments
Date Mon, 06 Feb 2006 16:11:43 GMT
Bryan Pendleton <bpendleton@amberpoint.com> writes:

> 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!

Thanks for a very thorough review, Bryan! I'll try to answer your
questions below. Hopefully, they make things clearer. Trying to find
the answers made things clearer for me, at least. :)

> 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.

Someone from IBM can problably answer this one.

> - 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.

No, the server doesn't communicate what it decided. According to the
spec, the server decides what to to based on cursor type. It says
nothing about what the server should decide, but it includes an
example of what the server could choose to decide. The current
implementation interprets SERVER_CHOICE as NO for all cursor types
(after Philip's fix to DERBY-213). Since the server doesn't tell the
client how it interprets SERVER_CHOICE, a client which specifies
SERVER_CHOICE must do one of the following:

  1) Check the product id and version of the server and guess based on
     its knowledge about that particular version of the server, or

  2) Assume that the server doesn't close cursors implicitly, but be
     prepared to handle error conditions that might occur if the
     server chooses to close.

>     - 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.

DRDA vol 3, p. 680:

  If the query is scrollable, the target server does not implicitly
  close the query regardless of the QRYCLSIMP value specified which is
  ignored. Otherwise, the QRYCLSIMP value results in one of the
  following behaviors: (...)

YES means that the server should close the cursor implicitly if it is
not scrollable. SERVER_CHOICE means that the server might or might not
close the cursor implicitly.

> - 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.

No, my implementation does not expect the server to make a decision
based on cursor type. The client sends QRYCLSIMP=YES in all OPNQRY
commands and expects the server to behave as specified by DRDA. I.e.,
if the cursor is scrollable ignore the parameter as the spec says, and
if the cursor is not scrollable always close the cursor implicitly.

The distinction between scrollable and non-scrollable cursors is
defined by DRDA for all values of QRYCLSIMP, not for SERVER_CHOICE
only.

> - 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.

I think this line was changed in the mentioned fix to DERBY-213, and
it's within the spec. Here's what the spec says about SERVER_CHOICE:

DRDA vol 3, p. 680:

  The target server determines whether to implicitly close the query
  or not upon SQLSTATE 02000 based on the cursor type. For instance,
  the target server may choose to implicitly close all non-scrollable
  cursors except those declared with the HOLD attribute.

Note that it says "for instance". Since it's just an example, there's
nothing preventing the server from making another choice.

> 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.

Well, even if they don't send SERVER_CHOICE explicitly, they might
skip the QRYCLSIMP parameter since it's optional. I haven't found a
that the spec says it is so, but I think it's reasonable that no
QRYCLSIMP parameter is interpreted as SERVER_CHOICE (that's what the
network server does). Since the current client driver does not send
QRYCLSIMP, we implicitly get SERVER_CHOICE from old clients. Changing
the way we interpret SERVER_CHOICE might cause a regression on
DERBY-213.

> - 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?

Again, the spec says "for instance". It confused me too for a while,
until I realized that it was just provided as an example to indicate
what the server *could* do, not what is *should* do.

> - 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 didn't like this part of the patch myself. The lines you are
referring to just define a constant to use instead of 'if (sqlcode ==
100)' as the current code does. The most important reason why I don't
like it, is that the DRDA spec says that the SQL state is 02000, but
the SQL code is only defined to be a value that the server
decides. This value happens to be 100 in Derby. However, I didn't want
to go through the code and replace every check for sqlcode==100 with
sqlstate.equals("02000"). I'll improve the comment, though.

> - 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?

EXCSQLSTT doesn't have the QRYCLSIMP parameter, so the client can't
request implicit closing.

> 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?

DRDAStatement.isRSCloseImplicit() doesn't check whether the result set
was requested with OPNQRY or EXCSQLSTT, but it checks the value of
DRDAResultSet.qryclsimp which is not set for EXCSQLSTT. Since the
server doesn't close implicitly unless qryclsimp is YES, EXCSQLSTT
result sets won't be closed.

However, the code could have been better written, e.g., when using
PRPSQLSTT/EXCSQLSTT to create a new DRDAResultSet,
DRDAResultSet.qryclsimp is not initialized, so the value is 0 which is
equal to SERVER_CHOICE. I'll make sure the variable is explicitly
initialized to NO on PRPSQLSTT/EXCSQLSTT. Thanks for catching this.

> Do you think that this
> is a place where having an extra regression test in the test suite would help?

lang/procedure.java already fails if the opposite is the case (client
thinks it is closed, but server hasn't closed it), so I could easily
add an extra test there.

> - 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?

I needed to be able to call ResultSet.markClosedOnServer() from
NetCursor, but couldn't because it had package access. Then I noticed
the method setCloseOnlyStateForSingletonCursors() which was public and
just forwarded the call to markClosedOnServer(). However, the method
was undocumented and not used in the code. Inspired by Dan's comment
in
http://mail-archives.apache.org/mod_mbox/db-derby-dev/200512.mbox/%3c43A9AA7F.1010808@debrunners.com%3e
I decided that it was better to make markClosedOnServer() public and
document it, and just delete the two unused 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?

Yes, that's 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?

Good idea, I'll try that.

> - 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 don't agree. I would say that this is a case which happens quite
often in real life. If someone writes a query and expects one result,
as in

   SELECT * FROM employees WHERE id = ?

most people would write

   if (rs.next()) {
     // do something
   }

In this case, the server sends one row followed by SQLSTATE 02000 (end
of data). Since next() is called only once, the client doesn't know
that it has received end of data, so it has to send CLSQRY to the
server. By scanning the data buffer, the client can see if it has
received end of data, in which case it doesn't have to send
CLSQRY. Scanning the data buffer is a light-weight operation compared
to sending a message to the server.

There is one situations where the performance can be reduced. That's
when you execute a query where the entire result set does not fit into
one message and you don't fetch all data returned from the query. In
that case, you might scan the data buffer just to find out that you
have to send CLSQRY anyway.

Anyway, according to the DRDA spec the server should send an QRYNOPRM
(Query Not Open) error message if it receives CLSQRY after the cursor
is closed, hence we have to check whether end of data is
received. Alteratively, we could handle QRYNOPRM in close(), but then
it would be difficult to distinguish real errors from expected errors.

> - 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.

Probably a good idea. I'll see what I can do.

>   - 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.

It would be tricky to write such a test. How do we provoke an error on
the implicit close?

Anyway, it seems like DRDAConnThread.processCommands() will catch
errors when prefetching data for the reply to CNTQRY. If an error
occurs (including close errors), the server returns an error code
instead of data, which will make execute() fail on the client.

>   - 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.

As you said, this would indicate a client bug, so I guess it is
difficult to write a test for it. Funny you should ask,
though. According to the spec, the server should send a QRYNOPRM
message back to the client, but that's not what it does. From
DRDAConnThread.processCommands():

    if (stmt.wasExplicitlyClosed())
    {
        // JCC still sends a CLSQRY even though we have
        // implicitly closed the resultSet.
        // Then complains if we send the writeQRYNOPRM
        // So for now don't send it
        // Also metadata calls seem to get bound to the same
        // PGKNAMCSN, so even for explicit closes we have
        // to ignore.
        //writeQRYNOPRM(CodePoint.SVRCOD_ERROR);
        pkgnamcsn = null;
    }

It doesn't seem like JCC is very DRDA compilant. :(

-- 
Knut Anders


Mime
View raw message