|
I have attached a patch (
prefetching on executeQuery(). Derbyall (including JCC tests) passed on Solaris 10 x86/Sun JVM 1.4.2 (a couple of failures, but they are also seen in Ole's nightly regression tests). I hope the size of the patch doesn't scare off potential reviewers. The actual code changes are very small. 95% of the patch is just updating the master files for the Wisconsin test. Below is a description of the changes. Thanks, Knut Anders MOTIVATION Derby (in client/server mode) needs more round trips to the server than other databases to execute select operations. While other database drivers prefetch data when calling PreparedStatement.executeQuery(), Derby does not fetch data until ResultSet.next() is called. If data is sent in the reply to PreparedStatement.executeQuery(), one round trip is saved per select operation and the performance will increase. PROPOSED SOLUTION The DRDA protocol lists some command sequences that require query data (QRYDTA) to be returned together with the reply to an open query (OPNQRY) command. None of these command sequences are currently in use by the Derby client driver or JCC, and the network server does not support them. Additionally, the DRDA protocol specifies command sequences for which the server can choose whether it includes QRYDTA in the reply or not. These sequences are used by the client driver, but the network server chooses not to send QRYDTA. What I propose, is that we change the network server's behaviour in the optional case, so that it includes QRYDTA when it replies to an OPNQRY command. I do not propose to implement support for the required case, since the clients do not currently send those commands to the server. IMPLEMENTATION The client driver (and JCC) already supports OPNQRYRM messages with QRYDTA attached, so only the network server needed to be modified. In the section of DRDAConnThread.processCommands() that processed OPNQRY commands, this comment was found: // We could send QRYDTA here if there's no LOB data // in the result set, and if we are using LMTBLKPRC, as // allowed by drda spec, as an option. At that spot I inserted code for writing QRYDTA similar to what processCommands() does for CNTQRY: if (stmt.getQryprctyp() == CodePoint.LMTBLKPRC) { // The DRDA spec allows us to send // QRYDTA here if there are no LOB // columns. DRDAResultSet drdars = stmt.getCurrentDrdaResultSet(); try { if (drdars != null && !drdars.hasLobColumns()) { writeQRYDTA(stmt, true); } } catch (SQLException sqle) { cleanUpAndCloseStatement(stmt, sqle, writerMark); } } All other changes to the code were just minor adjustments to make writeQRYDTA() and writeFDODTA() work with OPNQRY (they contained some code which assumed that they were responding to a CNTQRY). REGRESSION TESTING No explicit regression tests were added, since the amount of regression test failures caused by this change was so huge that a regression back to the old behaviour will not likely go unnoticed. For instance, the protocol test (derbynet/testProtocol.java) had to be changed to expect QRYDTA when it sends OPNQRY to the server, and it will fail if the network server changes are reverted. Below is a list of changes that had to be made to the regression tests: * jdbcapi/parameterMetaDataJdbc30.java * This test tried to compile an expression containing "WHERE x LIKE ? ESCAPE ?", set the escape sequence to an empty string and execute the query, but it did not call ResultSet.next() to fetch data. An empty string is not a valid escape sequence, so when prefetching was enabled the test failed. Failure was fixed by calling ResultSet.next() to ensure that the invalid escape sequence would be caught in all frameworks, and a message will be printed if we do not get the expected exception. * jdbcapi/setTransactionIsolation.java * Some test cases where a statement was executed without calling ResultSet.next() failed because the statement execution plan text differed. The difference was related to the prefetching which would cause a larger number of rows and pages seen. The execution plan did not change. Additionally, some two test cases failed because of a lock timeout. This was expected and caused by the prefetching. (You do not get a lock timeout until you actually try to fetch the data.) Fixed by updating master files for DerbyNet and DerbyNetClient. * jdbcapi/resultset.java * This test failed because some column headers were not printed. The failure was caused by the prefetching. Exceptions that were expected when calling ResultSet.next() were thrown when calling Statement.executeQuery() instead, leading to slightly different output. Updated master files for DerbyNet and DerbyNetClient. * derbynet/testProtocol.java * Test failed because it just expected OPNQRYRM and QRYDSC in response to OPNQRY with LMTBLKPRC and no LOBs. Had to add QRYDTA to the expected response in values1.inc. * lang/scrollCursors1.sql * One statement execution plan text changed because of prefetching (more rows seen). Updated master files for DerbyNet and DerbyNetClient. * lang/supersimple.sql * In queries that were expected to fail, column headers were not printed because prefetching caused the query to fail earlier. Updated master files for DerbyNet and DerbyNetClient. * lang/wisconsin.java * Many statement execution plan texts had changed. The plans still were the same, but the number of rows/pages seen increased since prefetching caused data to be fetched even though the test closed the cursors without reading data. Updated master files for DerbyNet and DerbyNetClient. * lang/forupdate.sql * Two occurrences of this error message: ERROR 42X23: Cursor SQL_CURLH000C1 is not updatable. were replaced with this message: ERROR 42X30: Cursor 'SQL_CURLH000C1' not found. Verify that autocommit is OFF. The failure was caused by prefetching causing all data from the forward-only/read-only cursor to be fetched, combined with implicit closing of the cursor on the server side. The failure is expected. Fixed by updating master file for DerbyNetClient. Created a sub-task (
Uploading a new patch (
removed. For the network server code, the patch is identical to the v1 patch. See the description of that patch for details. The client (both Derby and JCC) handles the new behaviour without code changes. The test changes made by this patch are: lang/supersimple.sql: Some queries which are expected to fail, don't print column names in DerbyNet and DerbyNetClient (because the failure is exposed earlier and the result set metadata isn't sent). Removed column names from the canons. lang/forupdate.sql: Some queries which failed with "Cursor not updatable" now fail with "Cursor not found". This is actually caused by this patch in combination with the patch for forward-only, read-only cursor is closed on the server because all rows are read, and there's no way to move backwards. The error message could probably have been more accurate, but it's not incorrect, and I believe most users will use ResultSet.deleteRow(), not "delete from t where current of ...". (ResultSet.{delete,insert,update}Row does say the concurrency of the result set is read-only.) Updated canons with the changed error messages. derbynet/testProtocol.java: In the file 'values1.inc', which is used frequently by the protocol test, the OPNQRYRM is expected to be followed by a QRYDSC only. With this patch, it is followed by a QRYDSC and a QRYDTA. Added this to the file, and also changed the test so that it checked that the codepoints were correct (the original test just did skipDss a number of times). Hi Knut Anders.
This diff is a lot easier to read now that I read through the changes and had a couple questions I wanted to ask: 1) I don't really understand why we have to pass the "opnqry" argument down to writeQRYDTA and writeFDODTA. I read your comments and they make sense at a "detail" level, but I think I'm missing the bigger picture. It seems to me like writeQRYDTA and writeFDODTA should not have to care whether they were called in response to a CNTQRY or in response to a OPNQRY, and when I stare at the detailed changes to writeFDODTA I'm puzzled: - when called during processing of an OPNQRY, why would it be wrong to call positionCursor() - when called during processing of an OPNQRY, doesn't stmt.getQryrtndta() return false? I think what I'm trying to suggest is that it seems like it would be cleaner to make positionCursor() and stmt.getQryrtndta() "do the right thing" when called during OPNQRY, rather than passing the flag through, since that seems like it would make the code overall more robust and clear. (That is, the "if" statements in writeFDODTA are already too complex for my taste, so I'm hoping not to add any more conditions to them). 2) I don't understand what's going on with the change to DRDAStatement.java. It seems like there are various configuration settings (qryrowset, blksize, maxblkext, etc.) which are being tracked both in the statement and in the result set, and you're changing things so that when these options are passed in an OPNQRY call, they will now affect not only the particular result set for that query, but also the overall statement? I think I'm just puzzled by this change. It doesn't seem to be directly related to the other work you're doing, except that it involves OPNQRY processing, and I'm concerned that I don't really understand the implications of setting these various variables on the statement object as opposed to on the result set object. Can you expand upon the reasoning behind the DRDAStatement change, and also help me understand why we have these variables in both the statement and the result set? Thank you for looking at the patch, Bryan! Your comments are valuable,
as always. I have tried to answer your questions below. > Bryan Pendleton commented on > --------------------------------------- > 1) I don't really understand why we have to pass the "opnqry" > argument down to writeQRYDTA and writeFDODTA. I read your comments > and they make sense at a "detail" level, but I think I'm missing the > bigger picture. It seems to me like writeQRYDTA and writeFDODTA > should not have to care whether they were called in response to a > CNTQRY or in response to a OPNQRY, and when I stare at the detailed > changes to writeFDODTA I'm puzzled: > - when called during processing of an OPNQRY, why would it be > wrong to call positionCursor() positionCursor() uses the value of QRYSCRORN, which is not part of the OPNQRY command. After an OPNQRY, DRDAResultSet.qryscrorn is unintialized (zero), and it is not accepted by positionCursor(). > - when called during processing of an OPNQRY, doesn't > stmt.getQryrtndta() return false? Yes, it does. And that is the problem. I realize now that my comment wasn't quite clear. The comment said: If we were asked not to return data (QRYRTNDTA) ... It should have said: If we were asked not to return data (QRYRTNDTA false) ... The original code has this assignment: boolean noRetrieveRS = (rs != null && !stmt.getQryrtndta()); Since qryrtndta is false when processing OPNQRY, noRetrieveRS is true, and no results are returned. Therefore, the test for OPNQRY was needed to make it work. > I think what I'm trying to suggest is that it seems like it would be > cleaner to make positionCursor() and stmt.getQryrtndta() "do the > right thing" when called during OPNQRY, rather than passing the flag > through, since that seems like it would make the code overall more > robust and clear. I agree. I think the best way is to set the values of DRDAResultSet.qryrtndta and DRDAResultSet.qryscrorn to something meaningful on OPNQRY. That should be safe, since the values are overwritten when a CNTQRY is received. > 2) I don't understand what's going on with the change to > DRDAStatement.java. It seems like there are various configuration > settings (qryrowset, blksize, maxblkext, etc.) which are being > tracked both in the statement and in the result set, and you're > changing things so that when these options are passed in an OPNQRY > call, they will now affect not only the particular result set for > that query, but also the overall statement? > > I think I'm just puzzled by this change. It doesn't seem to be > directly related to the other work you're doing, except that it > involves OPNQRY processing, and I'm concerned that I don't really > understand the implications of setting these various variables on > the statement object as opposed to on the result set object. > > Can you expand upon the reasoning behind the DRDAStatement change, > and also help me understand why we have these variables in both the > statement and the result set? Right, I didn't explain that change very well... First, I'll explain why these changes were needed: The current implementation does not set default block size (and some other options) for the DRDAStatement on an OPNQRY command. Actually, the only command that sets these options is EXCSQLSTT. For the old behaviour with no pre-fetching, this is okay since the block size for the result set is set on each CNTQRY command. However, it is not okay with pre-fetching. What happens is 1) DRDAConnThread.processCommands() calls parseOPNQRY(), which sets the block size of the current DRDAResultSet to ~32K. 2) DRDAConnThread.processCommands() calls DRDAStatement.execute(), which (indirectly) calls setRsDefaultOptions(). Since the default block size of the DRDAStatement is not set, the block size of the current DRDAResultSet will be reset to zero. 3) When DRDAConnThread.processCommands() tries to pre-fetch rows it only fetches one row, since it thinks that the block is full. If we set the default block size for the statement on OPNQRY, the result set won't get its block size reset when the statement is executed, and we can fill the entire block with pre-fetched rows. Only the block size was necessary to get the pre-fetching working, but I felt it was cleaner to update all the default values rather than just one. (Actually, I see that I forgot one variable. Will fix that one.) Now, here's why I think this change is correct: a) There is never more than one open result set per statement, so changing the defaults for the statement does not affect any other open result set. b) The default values are only used in setRsDefaultOptions(). All code paths that end up calling setRsDefaultOptions() go through DRDAStatement.execute(). DRDAStatement.execute() is only invoked from within parseEXCSQLSTT() and after parseOPNQRY(). Since parseEXCSQLSTT() already updates the defaults, it won't see any changes made by OPNQRY. You also asked why the variables were needed in both DRDAStatement and DRDAResultSet. I don't see any good reason for having two copies of these variables. I think the code would be cleaner if we got rid of the ones in DRDAStatement. But that's another patch... ;) Uploading new patch (
- renamed clean-up method for Beetle 4758 from cleanUpAndCloseStatement to cleanUpAndCloseResultSet - assigned default values to QRYRTNDTA, QRYSCRREL and QRYROWNBR on OPNQRY, so that writeQRYDTA() and writeFDODTA() can be used without changes before CNTQRY has been called Derbyall ran cleanly on Sun JVM 1.5, Solaris 10 x86. Thank you for investigating my feedback. This has become a very elegant change, and I have no further suggestions to make. Great work!
Thanks again for your feedback, Bryan!
Committed revision 394859. I noticed that on the day of this checkin, lang/simpleScroll.sql, and lang/scrollCursors1.sql with JCC 2.5 (56) started failing with a protocol error.
The test is ok with JCC 2.4(17), JCC 2.5 (36) and JCC 2.6 (90) are all ok. I am not sure yet if this is some sort of regression with our DRDA handling or a bug with this particular verison of JCC. I will post more info as I have it. Here is the diff for scrollCursors1 *** Start: scrollCursors1 jdk1.5.0_02 DerbyNet derbynetmats:jdbc20 2006-04-18 23:37:03 *** 392a393 > ERROR 58017: The DDM parameter value is not supported. DDM parameter code point having unsupported value : 0x2102 394 del I 395 del < ----- 396 del < 2 396a395 > IJ ERROR: Unable to establish cursor 398 del < I 399 del < ----- 400 del < 3 400a397 > IJ ERROR: Unable to establish cursor 402 del < 1 row inserted/updated/deleted 402a399 > ERROR (no SQLState): invalid operation: connection closed I think we probably need to check the "Existing application Impact" box on this issue and provide a release note because I think this is one of those cases where we intentionally introduced the following behaviour changes:
1) Queries may fail earlier. Instead of failing on the ResultSet next() call, they may now fail on execute()/executeQuery(). Applications need to be prepared to handle this. 2) Prefetching may impact locking behavior. Locks may be acquired earlier or may be acquired where they never were before, promoted to table locks etc. 3) This introduces a new difference I think between embedded and network behavior Does this sound right? If so I can take a stab and a release note. Is there a workaround if prefetching causes users any trouble on upgrade? Kathey Marsden wrote:
> 1) Queries may fail earlier. Instead of failing on the ResultSet > next() call, they may now fail on execute()/executeQuery(). > Applications need to be prepared to handle this. This is correct. However, applications should already be prepared to handle errors on execute()/executeQuery(), so I hope this shouldn't cause too much trouble. > 2) Prefetching may impact locking behavior. Locks may be acquired > earlier or may be acquired where they never were before, > promoted to table locks etc. This is partly correct. Locks for the first chunk of pre-fetched rows are aquired on execute/executeQuery instead of on the first call to next(). The locks "may be acquired where they never were before", but that is only the case when there is no call to next() after the execute. Although a lot of our tests do this (in order to test that the statement compiles or to print the query execution plan), I fail to see why an application would execute a SELECT statement but not read any of the data. I do not believe locks will be promoted to table locks because of the pre-fetching. The number of rows pre-fetched when invoking executeQuery() is exactly the same as the number of rows that would be pre-fetched on the first call to next() with an old server. > 3) This introduces a new difference I think between embedded and > network behavior Correct. > Does this sound right? If so I can take a stab and a release > note. Thanks, that would be great! > Is there a workaround if prefetching causes users any trouble > on upgrade? The best work-around is: Do not execute SELECT statements just for fun! :) Seriously, if a select statement is executed, but the results are never read, it should probably not have been executed in the first place. Also, it might help to move the executeQuery() closer to the first call to next() (that is, don't do too much work between executeQuery() and next()). There is no simple way to disable the pre-fetching. The only ways I know of are using an updateable result set or including a LOB column in the query. RELEASE NOTE:
Pre-fetching with Network Client/Server is a performance optimization that eliminates a round trip to the server. Essentially the first ResultSet next() call is executed on the Statement executeQuery() or execute(). Applications are typically prepared to handle this but the following issue may be seen, PROBLEM Queries may fail earlier and locks may be aquired earlier when executing queries. Location where errors occur embedded environment is different. SYMPTOM Errors that happen as part of the normal execution path are moved earlier. For example, code to execute a query, with executeQuery() retrieve the result set metadata and then perform a next() might fail with a lock timeout on executeQuery() instead of next(). Locking changes are observed. CAUSE Pre-fetching moves execution of retrieval of data earlier for network client/server configurations. SOLUTION This was an intentional behavior change to improve performance. No Derby product solution is offered. WORKAROUND Application code needs to be changed to adjust error handling if needed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Improvement
Closed
Minor
Client driver: Pre-fetch data on executeQuery()
This seems easier than I first thought. In theory, at least... When
the server receives an OPNQRY (Open Query) it is supposed to respond
with an OPNQRYRM (Open Query Reply Message), QRYDSC (Query Answer Set
Description), and optionally (and with some restrictions) QRYDTA
(Query Answer Set Data).
Right now, the server doesn't return QRYDTA in the response to OPNQRY,
but I found this comment in DRDAConnThread.processCommands():
writeQRYDSC(stmt, false);
// We could send QRYDTA here if there's no LOB data
// in the result set, and if we are using LMTBLKPRC, as
// allowed by drda spec, as an option.
I am not convinced that the comment about LMTBLKPRC is correct, I
think it is also allowed for FIXROWPRC. Will look more into that.
Anyway, I have enabled sending of QRYDTA for LMTBLKPRC and that seems
to work for some simple tests I have run. A lot of failures in
derbynetmats and derbynetclientmats, though. Many of the failures are
caused by changes in the output from SYSCS_GET_RUNTIMESTATISTICS()
(pretty huge diffs). Will have to look more at the failures to see
which ones are real failures.
So far, I haven't had to make any changes on the client side.