Derby
  1. Derby
  2. DERBY-3192

Cache session data in the client driver

    Details

    • Bug behavior facts:
      Performance

      Description

      The reason for doing this is to avoid a rather
      substantial performance hit observed when the client driver is used
      together with an appserver that uses connection pooling. There are two
      problems:

      1) The connection pool will compare the isolation level it has
      stored for the connection with the value returned from
      Connection.getTransactionIsolation() each and every time someone
      requests a new connection from the pool.

      2) The users of the connection pool (ab)use it to avoid having to keep
      track of their current connection. So each time a query needs to be
      executed a call to the connection pool's getConnection() method is
      made. Getting a connection from the connection pool like this also
      means that a new PreparedStatement must be prepared each time.

      The net result is that each query results in the following sequence:

      getConnection()
      getTransactionIsolation() --> roundtrip + lookup in server's statement cache

      prepareStatment() --> roundtrip + lookup in server's statement cache

      executeQuery() --> roundtrip

      Arguably this is a "user error" but when suggesting this I'm kindly
      informed that this works "just fine" with other datbases (such as
      PostgreSQL and ORACLE).

      The reason why it works is that these databases do statement caching
      in the driver. I've tried to implement a very (too) simple statement
      cache in Derby's client driver and to re-enable caching of the
      isolation level (see
      https://issues.apache.org/jira/browse/DERBY-1148). With these changes
      I observe a marked performance improvement when running with appserver
      load.

      A proper statment cache cannot be implemented without knowing what the
      current schema is. If the current schema has changed since the
      statement was prepared, it is no longer valid and must be evicted from
      the cache.

      The problem with caching both the isolation level and the current schema in
      the driver is that both can change on the server without the client
      detecting it (through SQL and XA and possibly stored procedures).

      I think this problem can be overcome if we piggy-back the information we would
      like to cache on messages going back to the client. This can be done by
      utilizing the EXCSQLSET DRDA command. According to the DRDA spec (v4, volume 3,
      page 359-360) it is possible to add one or more SQLSTT objects after SQLCARD in the reply,

      I think it would be possible to cache additional session information when this becomes relevant. It
      would also be possible to use EXCSQLSET to batch session state changes
      going from the client to the server.

      1. derby-3192-test.v1.stat
        0.3 kB
        Dyre Tjeldvoll
      2. derby-3192-test.v1.diff
        31 kB
        Dyre Tjeldvoll
      3. derby-3192-test.fup2.diff
        10 kB
        Dyre Tjeldvoll
      4. derby-3192-test.fup1.diff
        10 kB
        Dyre Tjeldvoll
      5. derby-3192-mark2.v8.diff
        46 kB
        Dyre Tjeldvoll
      6. derby-3192-mark2.v7.diff
        46 kB
        Dyre Tjeldvoll
      7. derby-3192-mark2.v6.diff
        45 kB
        Dyre Tjeldvoll
      8. derby-3192-mark2.v5.diff
        45 kB
        Dyre Tjeldvoll
      9. derby-3192-mark2.v4.diff
        45 kB
        Dyre Tjeldvoll
      10. derby-3192-mark2.v3.diff
        43 kB
        Dyre Tjeldvoll
      11. derby-3192-mark2.v2.diff
        44 kB
        Dyre Tjeldvoll
      12. derby-3192-mark2.v1.diff
        43 kB
        Dyre Tjeldvoll
      13. derby-3192-fup.v1.diff
        2 kB
        Dyre Tjeldvoll
      14. derby-3192.prelim1.diff
        83 kB
        Dyre Tjeldvoll

        Issue Links

          Activity

          Dyre Tjeldvoll created issue -
          Dyre Tjeldvoll made changes -
          Field Original Value New Value
          Assignee Dyre Tjeldvoll [ dyret ]
          Hide
          Dyre Tjeldvoll added a comment -

          In the interest of having test-driven and incremental development I'm starting my work on this issue
          by attaching a patch for a new JUnit test. The test runs ok by itself and as a part of jdbcapi. I have not run it as part of suites.All.

          Currently, only isolation level changes are tested.

          If anyone knows about other ways of changing the isolation level that should be part of this test, I would love to hear about it. (Other comments are also appreciated).

          Show
          Dyre Tjeldvoll added a comment - In the interest of having test-driven and incremental development I'm starting my work on this issue by attaching a patch for a new JUnit test. The test runs ok by itself and as a part of jdbcapi. I have not run it as part of suites.All. Currently, only isolation level changes are tested. If anyone knows about other ways of changing the isolation level that should be part of this test, I would love to hear about it. (Other comments are also appreciated).
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-test.v1.diff [ 12369820 ]
          Attachment derby-3192-test.v1.stat [ 12369821 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Forgot to set patch available when I uploaded the patch. if there are no objections, I plan to commit this patch in a couple of days.

          Show
          Dyre Tjeldvoll added a comment - Forgot to set patch available when I uploaded the patch. if there are no objections, I plan to commit this patch in a couple of days.
          Dyre Tjeldvoll made changes -
          Derby Info [Patch Available]
          Hide
          Dyre Tjeldvoll added a comment -

          Committed revision 597352.

          Show
          Dyre Tjeldvoll added a comment - Committed revision 597352.
          Dyre Tjeldvoll made changes -
          Derby Info [Patch Available]
          Hide
          Knut Anders Hatlen added a comment -

          Hi Dyre,

          I think it's a very good idea to write the test up front, and the test looks good. I'm not sure, but I think maybe the test needs to be excluded when running JSR-169 tests since the stored procedures use DriverManager.

          Some nits:

          Some of the test cases have code which looks like this:
          + Connection c = getConnection();
          + Statement s = c.createStatement();
          and
          + Connection c = getConnection();
          + PreparedStatement ps = c.prepareStatement("SELECT * FROM " + table,

          It's probably better to use the helper methods create/prepareStatement() in JDBCBaseTestCase, since they will automatically perform clean-up in tearDown().

          It seems like the assertEquals() calls have swapped expected and actual value (correct order is assertEquals(expected, actual)).

          Show
          Knut Anders Hatlen added a comment - Hi Dyre, I think it's a very good idea to write the test up front, and the test looks good. I'm not sure, but I think maybe the test needs to be excluded when running JSR-169 tests since the stored procedures use DriverManager. Some nits: Some of the test cases have code which looks like this: + Connection c = getConnection(); + Statement s = c.createStatement(); and + Connection c = getConnection(); + PreparedStatement ps = c.prepareStatement("SELECT * FROM " + table, It's probably better to use the helper methods create/prepareStatement() in JDBCBaseTestCase, since they will automatically perform clean-up in tearDown(). It seems like the assertEquals() calls have swapped expected and actual value (correct order is assertEquals(expected, actual)).
          Hide
          Dyre Tjeldvoll added a comment -

          Hi Knut,
          thank you for comments. You have valid points and I plan to address them soon.

          Wrt. to JSR-169; I think all test cases depend on at least one stored procedure that uses DriverManager (verifyCachedIsolation(), which is used by all test cases, use such a stored procedure). So what is the best way of disabling the entire test when running with JSR-169? I guess I could let suite() return only the C/S decorated suite, since functionality being tested only is used in C/S mode, but perhaps there is a more suitable way?

          Show
          Dyre Tjeldvoll added a comment - Hi Knut, thank you for comments. You have valid points and I plan to address them soon. Wrt. to JSR-169; I think all test cases depend on at least one stored procedure that uses DriverManager (verifyCachedIsolation(), which is used by all test cases, use such a stored procedure). So what is the best way of disabling the entire test when running with JSR-169? I guess I could let suite() return only the C/S decorated suite, since functionality being tested only is used in C/S mode, but perhaps there is a more suitable way?
          Hide
          Knut Anders Hatlen added a comment -

          You could wrap the adding of tests in the suite() method with "if (JDBC.vmSupportsJDBC3()) ...", so that it only returns an empty test suite for JSR-169. It seems like other tests that don't run under JSR-169 additionally add themselves to the main test suite from inside a similar if statement in jdbcapi._Suite.suite().

          Show
          Knut Anders Hatlen added a comment - You could wrap the adding of tests in the suite() method with "if (JDBC.vmSupportsJDBC3()) ...", so that it only returns an empty test suite for JSR-169. It seems like other tests that don't run under JSR-169 additionally add themselves to the main test suite from inside a similar if statement in jdbcapi._Suite.suite().
          Hide
          Dyre Tjeldvoll added a comment -

          I attached .fup1.diff which addresses Knut's comments.

          Show
          Dyre Tjeldvoll added a comment - I attached .fup1.diff which addresses Knut's comments.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-test.fup1.diff [ 12370078 ]
          Hide
          Knut Anders Hatlen added a comment -

          Looks good. +1 to commit.

          I noticed that you had a couple of System.out.println() statements commented out. Just in case you're not already aware of it, the println() method in BaseTestCase gives you an easier way to enable/disable debug printing without recompiling the test. It is normally silent, but if you set derby.tests.debug to true, it starts printing the messages.

          Show
          Knut Anders Hatlen added a comment - Looks good. +1 to commit. I noticed that you had a couple of System.out.println() statements commented out. Just in case you're not already aware of it, the println() method in BaseTestCase gives you an easier way to enable/disable debug printing without recompiling the test. It is normally silent, but if you set derby.tests.debug to true, it starts printing the messages.
          Hide
          Dyre Tjeldvoll added a comment -

          That is good to know. I expect that those printlns can be removed completely one day, but since I'm still developing the feature being tested it is very useful to have some more info about what the test is doing, so that's why I kept them.

          Show
          Dyre Tjeldvoll added a comment - That is good to know. I expect that those printlns can be removed completely one day, but since I'm still developing the feature being tested it is very useful to have some more info about what the test is doing, so that's why I kept them.
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching *.fup2.diff which does s/System.out.println/println/g

          Show
          Dyre Tjeldvoll added a comment - Attaching *.fup2.diff which does s/System.out.println/println/g
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-test.fup2.diff [ 12370114 ]
          Dyre Tjeldvoll made changes -
          Link This issue depends on DERBY-3198 [ DERBY-3198 ]
          Hide
          Dyre Tjeldvoll added a comment -

          DERBY-3198 also uses writeSetSpecialRegister() so it suffers from same problem.

          Show
          Dyre Tjeldvoll added a comment - DERBY-3198 also uses writeSetSpecialRegister() so it suffers from same problem.
          Dyre Tjeldvoll made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Dyre Tjeldvoll added a comment -

          I've attached a preliminary patch (prelim1) which implements caching of the isolation level in the client driver. It is a fully working solution and all the tests pass, but there is a particular implementation issue which I especially would like to get feedback on:

          When executing a query with a scrollable result set a complete row will be fetched each time Statement.flowExecute() is called, even if this requires multiple round-trips. This means that trailing piggy-backed session data may end up being processed either by the code assembling a complete row for a scrollable result set, or by flowExecute (normal case). This will complicate the logic in because flowExecute needs to know if the session data was processed when the row was assembled, or if the session data is still waiting to be read.

          In the attached patch I've chosen to work around this problem by detecting split rows destined for a scrollable result set on the server, and then delay sending the session data until the final fragment of the row is sent. Doing it this way has three benefits:
          1) No piggy-backing logic needs to be added to the row-assembly logic
          2) No complication of flowExecute
          3) No unnecessary piggy-backing is done while a row is re-assembled

          The primary drawback is that the DRDAConnThread logic gets more complicated and normal DRDA flow is interrupted. Normally each DRDA request is receives a corresponding reply in next message going back to the client. In this patch the answer to a session info request is delayed until all fragments of a row have been sent. This breaks the normal DRDA chaining mechanism. The details can be seen by searching for comments beginning with 'TODO DT' in the patch.

          All review comments will be much appreciated.

          Show
          Dyre Tjeldvoll added a comment - I've attached a preliminary patch (prelim1) which implements caching of the isolation level in the client driver. It is a fully working solution and all the tests pass, but there is a particular implementation issue which I especially would like to get feedback on: When executing a query with a scrollable result set a complete row will be fetched each time Statement.flowExecute() is called, even if this requires multiple round-trips. This means that trailing piggy-backed session data may end up being processed either by the code assembling a complete row for a scrollable result set, or by flowExecute (normal case). This will complicate the logic in because flowExecute needs to know if the session data was processed when the row was assembled, or if the session data is still waiting to be read. In the attached patch I've chosen to work around this problem by detecting split rows destined for a scrollable result set on the server, and then delay sending the session data until the final fragment of the row is sent. Doing it this way has three benefits: 1) No piggy-backing logic needs to be added to the row-assembly logic 2) No complication of flowExecute 3) No unnecessary piggy-backing is done while a row is re-assembled The primary drawback is that the DRDAConnThread logic gets more complicated and normal DRDA flow is interrupted. Normally each DRDA request is receives a corresponding reply in next message going back to the client. In this patch the answer to a session info request is delayed until all fragments of a row have been sent. This breaks the normal DRDA chaining mechanism. The details can be seen by searching for comments beginning with 'TODO DT' in the patch. All review comments will be much appreciated.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192.prelim1.diff [ 12371731 ]
          Dyre Tjeldvoll made changes -
          Derby Info [Patch Available]
          Hide
          Knut Anders Hatlen added a comment -

          Hi Dyre,

          In DRDAConnThread.writeEXCSQLSETReply() I see this code:
          + else {
          + // At least one SQLSTT is required when rtnsetstt is true
          + writeSQLSTT("");

          However, DRDA V4, vol 3, p. 761 says this about RTNSETSTT:
          > No SQLSTT reply data object is returned by the target server if no special register has had its
          > setting modified on the current connection, regardless of the RTNSETSTT setting.

          Doesn't this mean that we shouldn't send SQLSTT if the cached session state hasn't changed?

          Show
          Knut Anders Hatlen added a comment - Hi Dyre, In DRDAConnThread.writeEXCSQLSETReply() I see this code: + else { + // At least one SQLSTT is required when rtnsetstt is true + writeSQLSTT(""); However, DRDA V4, vol 3, p. 761 says this about RTNSETSTT: > No SQLSTT reply data object is returned by the target server if no special register has had its > setting modified on the current connection, regardless of the RTNSETSTT setting. Doesn't this mean that we shouldn't send SQLSTT if the cached session state hasn't changed?
          Hide
          Knut Anders Hatlen added a comment -

          By the way, is the plan to have the caching enabled by default? Do you think it's worth the effort to make it optional? If I understand correctly, the caching will increase the size of a request from the client by about 30 bytes (this could perhaps be reduced by using a shorter SET statement string?), and there will be close to zero bytes extra on the reply unless the state has changed. This sounds like a negligible cost to me, but I think it would be a good idea to run a VALUES 1 test or something similar to verify that it doesn't degrade the general performance.

          Show
          Knut Anders Hatlen added a comment - By the way, is the plan to have the caching enabled by default? Do you think it's worth the effort to make it optional? If I understand correctly, the caching will increase the size of a request from the client by about 30 bytes (this could perhaps be reduced by using a shorter SET statement string?), and there will be close to zero bytes extra on the reply unless the state has changed. This sounds like a negligible cost to me, but I think it would be a good idea to run a VALUES 1 test or something similar to verify that it doesn't degrade the general performance.
          Hide
          Daniel John Debrunner added a comment -

          Is there a writeup that describes how this is implemented, kind of hard to review the patch otherwise.

          Show
          Daniel John Debrunner added a comment - Is there a writeup that describes how this is implemented, kind of hard to review the patch otherwise.
          Hide
          Dyre Tjeldvoll added a comment -

          @Knut1: I hope your interpretation of the DRDA spec is correct as
          I would prefer not sending an empty string when there is no info.

          @Knut2: The current version of the patch does not allow the
          caching to be turned off, but this could certainly be added. Your
          estimates of the overhead are accurate AFAICT. I did run some
          tests (d2911) to see if there was a perf impact and I could not
          see any degradation. I could re-run those tests with a final
          version of the patch and attach them here.

          @Dan: There is no writeup per se, only the following discussions
          on derby-dev (part of which is included in the Description
          field):

          http://www.nabble.com/Cache-session-data-in-the-client-driver--to13231212.html#a13231212

          and

          http://www.nabble.com/DRDA-confusion-to13495130.html#a13495130

          I could certainly provide a writeup. It didn't occur to me that one
          would be needed, since the plan was to use existing mechanisms
          that are currently used for similar
          purposes (setQueryTimeout). In retrospect, there are some
          unforeseen complications that mandate more explanation, but I'm
          guessing that they would not have been part of an initial writeup
          either...

          Show
          Dyre Tjeldvoll added a comment - @Knut1: I hope your interpretation of the DRDA spec is correct as I would prefer not sending an empty string when there is no info. @Knut2: The current version of the patch does not allow the caching to be turned off, but this could certainly be added. Your estimates of the overhead are accurate AFAICT. I did run some tests (d2911) to see if there was a perf impact and I could not see any degradation. I could re-run those tests with a final version of the patch and attach them here. @Dan: There is no writeup per se, only the following discussions on derby-dev (part of which is included in the Description field): http://www.nabble.com/Cache-session-data-in-the-client-driver--to13231212.html#a13231212 and http://www.nabble.com/DRDA-confusion-to13495130.html#a13495130 I could certainly provide a writeup. It didn't occur to me that one would be needed, since the plan was to use existing mechanisms that are currently used for similar purposes (setQueryTimeout). In retrospect, there are some unforeseen complications that mandate more explanation, but I'm guessing that they would not have been part of an initial writeup either...
          Hide
          Daniel John Debrunner added a comment -

          > In retrospect, there are some unforeseen complications that mandate more explanation, but I'm guessing that they would not have been part of an initial writeup either...

          It's not an initial writeup I was looking for, but some summary of the actual change being made.

          The description is this bug has a couple of places where it says "I think ....", so that doesn't really help a reviewer of the code. Did the implementor solve this issues using the ideas that followed the "I think", or was it handled some other way?

          It's also not clear from the description that the mechanism to be used is the same as setQueryTimeout, it's not mentioned until the last comment.

          I personally just don't know how to review a 1552 line patch without a good overview of is being attempted and how it is being attempted, I'm then just reading code that obviously does X without knowing if X was what is intended or not.

          These comments in this issue worry me:

          • "the caching will increase the size of a request from the client by about 30 bytes" - is this every request, seems too much to me?
          • "When executing a query with a scrollable result ..." - why is scrollable result set called out here, at a high level I can't see why the scrollability of a result set would affect caching of session state?

          Of course no-one has to provide a writeup, as long as a committer has confidence in a patch it can be committed, but producing a writeup has the muilti-purpose of adding to the community knowledge, potentially improving the quality of the code & reviews and often helps the implementor ensure they fully understand the changes they are making

          Show
          Daniel John Debrunner added a comment - > In retrospect, there are some unforeseen complications that mandate more explanation, but I'm guessing that they would not have been part of an initial writeup either... It's not an initial writeup I was looking for, but some summary of the actual change being made. The description is this bug has a couple of places where it says "I think ....", so that doesn't really help a reviewer of the code. Did the implementor solve this issues using the ideas that followed the "I think", or was it handled some other way? It's also not clear from the description that the mechanism to be used is the same as setQueryTimeout, it's not mentioned until the last comment. I personally just don't know how to review a 1552 line patch without a good overview of is being attempted and how it is being attempted, I'm then just reading code that obviously does X without knowing if X was what is intended or not. These comments in this issue worry me: "the caching will increase the size of a request from the client by about 30 bytes" - is this every request, seems too much to me? "When executing a query with a scrollable result ..." - why is scrollable result set called out here, at a high level I can't see why the scrollability of a result set would affect caching of session state? Of course no-one has to provide a writeup, as long as a committer has confidence in a patch it can be committed, but producing a writeup has the muilti-purpose of adding to the community knowledge, potentially improving the quality of the code & reviews and often helps the implementor ensure they fully understand the changes they are making
          Dyre Tjeldvoll made changes -
          Derby Info [Patch Available]
          Kristian Waagan made changes -
          Link This issue blocks DERBY-3313 [ DERBY-3313 ]
          Hide
          Dyre Tjeldvoll added a comment -

          I'm currently creating a writeup which describes the patch, but
          in the process I have discovered that the current solution for
          the previously mentioned multiple round-trip problem is
          broken.

          The multiple round-trip problem only appears
          for ((TYPE_SCROLL_SENSITIVE || TYPE_SCROLL_INSENSITIVE) &&
          CONCUR_READ_ONLY) result sets.

          The current solution will work correctly, only if fetchSize is 1
          or greater than the number of round-trips needed. If this
          criteria does not hold, the client will wait indefinitely for
          session data that the server has not yet sent. I guess the best
          way to avoid this is to extend 'flowToCompleteRowset()' so that
          it reads and sends session data.

          Show
          Dyre Tjeldvoll added a comment - I'm currently creating a writeup which describes the patch, but in the process I have discovered that the current solution for the previously mentioned multiple round-trip problem is broken. The multiple round-trip problem only appears for ((TYPE_SCROLL_SENSITIVE || TYPE_SCROLL_INSENSITIVE) && CONCUR_READ_ONLY) result sets. The current solution will work correctly, only if fetchSize is 1 or greater than the number of round-trips needed. If this criteria does not hold, the client will wait indefinitely for session data that the server has not yet sent. I guess the best way to avoid this is to extend 'flowToCompleteRowset()' so that it reads and sends session data.
          Hide
          Dyre Tjeldvoll added a comment -

          I've finally managed to finish the writeup I promised. It can be found at
          http://wiki.apache.org/db-derby/Derby3192Writeup

          Show
          Dyre Tjeldvoll added a comment - I've finally managed to finish the writeup I promised. It can be found at http://wiki.apache.org/db-derby/Derby3192Writeup
          Hide
          Knut Anders Hatlen added a comment -

          In the writeup, I read this:
          > Most of the overhead seems to come from the PKGNAMCSN instance variable which by itself uses 76 bytes

          It does however seem like DRDA allows this variable to be skipped.

          DRDA, ver. 4, vol 1, p 10:

          > PKGNAM, PKGNAMCSN, PKGNAMCT
          > The length is no longer fixed and is based on the lengths of the RDBNAM,
          > RDBCOLID, and PKGID contained therein. As of SQLAM Level 7, the
          > PKGNAMCSN instance variable is optional. If not specified, the PKGSN is
          > required to identify the package section number. The package name and
          > consistency token defaults to the last set of values specified on the connection.

          DRDA, ver. 4, vol 1, p 463:

          > CU15 The fully qualified package name and package consistency token are not required to be
          > specified on every SQL-related request. If the package name and consistency token are
          > not specified on a request, the last request that specified the package name and
          > consistency token is used to identify the package name and consistency token for the
          > request. The package section number is not optional and is required if the package
          > name and consistency token are not specified. If the package name and consistency
          > token were not specified on a previous request to establish the default, the request is
          > rejected by the application server with a conversational protocol error with the error
          > code set to X'20' (default package name not established).

          Show
          Knut Anders Hatlen added a comment - In the writeup, I read this: > Most of the overhead seems to come from the PKGNAMCSN instance variable which by itself uses 76 bytes It does however seem like DRDA allows this variable to be skipped. DRDA, ver. 4, vol 1, p 10: > PKGNAM, PKGNAMCSN, PKGNAMCT > The length is no longer fixed and is based on the lengths of the RDBNAM, > RDBCOLID, and PKGID contained therein. As of SQLAM Level 7, the > PKGNAMCSN instance variable is optional. If not specified, the PKGSN is > required to identify the package section number. The package name and > consistency token defaults to the last set of values specified on the connection. DRDA, ver. 4, vol 1, p 463: > CU15 The fully qualified package name and package consistency token are not required to be > specified on every SQL-related request. If the package name and consistency token are > not specified on a request, the last request that specified the package name and > consistency token is used to identify the package name and consistency token for the > request. The package section number is not optional and is required if the package > name and consistency token are not specified. If the package name and consistency > token were not specified on a previous request to establish the default, the request is > rejected by the application server with a conversational protocol error with the error > code set to X'20' (default package name not established).
          Hide
          Dyre Tjeldvoll added a comment -

          Thnak you, Knut. That is very good news!
          I may need some time to digest the details, though.

          Btw. You don't know if it is also legal to truncate/drop SQLCARD in the reply, do you?

          Show
          Dyre Tjeldvoll added a comment - Thnak you, Knut. That is very good news! I may need some time to digest the details, though. Btw. You don't know if it is also legal to truncate/drop SQLCARD in the reply, do you?
          Hide
          Knut Anders Hatlen added a comment -

          > You don't know if it is also legal to truncate/drop SQLCARD in the reply, do you?
          No, I don't know.

          I just noticed that the DRDA spec allows you to define product-unique extensions as long as you're still able to talk to a server/client which doesn't have the extension. I haven't studied the details, but you could take a look at section 2.5.5.4 in DRDA ver 4, vol 3. Perhaps we could have a custom OPNQRYRM/QRYDTA object which contained session state with minimal overhead?

          Show
          Knut Anders Hatlen added a comment - > You don't know if it is also legal to truncate/drop SQLCARD in the reply, do you? No, I don't know. I just noticed that the DRDA spec allows you to define product-unique extensions as long as you're still able to talk to a server/client which doesn't have the extension. I haven't studied the details, but you could take a look at section 2.5.5.4 in DRDA ver 4, vol 3. Perhaps we could have a custom OPNQRYRM/QRYDTA object which contained session state with minimal overhead?
          Hide
          Dyre Tjeldvoll added a comment -

          Thank you! This is indeed interesting.

          This really opens up the possibilities. But an asynchronous approach as you suggest (no polling, which is great) would require some handshake logic to figure out if both the client and the server supports this. I'm guessing that perhaps EXCSAT could be used for this somehow, but I don't quite understand how it works yet...

          Show
          Dyre Tjeldvoll added a comment - Thank you! This is indeed interesting. This really opens up the possibilities. But an asynchronous approach as you suggest (no polling, which is great) would require some handshake logic to figure out if both the client and the server supports this. I'm guessing that perhaps EXCSAT could be used for this somehow, but I don't quite understand how it works yet...
          Hide
          Knut Anders Hatlen added a comment -

          The server knows which version of the client it talks to. See impl.drda.AppRequester.getClientType()/greaterThanOrEqualTo(). This could be used to decide which kind of reply to send. Similarly, the client knows the version of the server (see NetDatabaseMetadata.computeFeatureSet_()).

          Show
          Knut Anders Hatlen added a comment - The server knows which version of the client it talks to. See impl.drda.AppRequester.getClientType()/greaterThanOrEqualTo(). This could be used to decide which kind of reply to send. Similarly, the client knows the version of the server (see NetDatabaseMetadata.computeFeatureSet_()).
          Hide
          Dyre Tjeldvoll added a comment -

          Ah, good. One less thing to worry about. I'm trying to understand how you can add 'product-specific' extensions (specifically code points) to DDM commands. I'm trying
          to understand the CODPNTDR section of vol3 (page 224):

          Codepoints consist of two values, a 1-hex index into a list of dictionaries and a 3-hex identifier
          unique within the referenced dictionary. The resolution of codepoints to class descriptors is
          illustrated in Figure 3-18 (on page 225).
          For Agent Level 3, the list of dictionaries is fixed, and specific index values are assigned as
          follows:
          • C-F: Reserved for product extensions (see EXTENSIONS (on page 381))
          • 4-B: Reserved for DDM Architecture
          • 3: DDM dictionary QDDADLD
          • 2: DDM dictionary QDDRDBD
          • 1: DDM dictionary QDDBASD
          • 0: DDM dictionary QDDPRMD

          Does that mean I can use codepoints in the range [0xC000-0xFFFF] to product-unique stuff? And use them as instance variables in OPNQRYRM/QRYDTA?

          Show
          Dyre Tjeldvoll added a comment - Ah, good. One less thing to worry about. I'm trying to understand how you can add 'product-specific' extensions (specifically code points) to DDM commands. I'm trying to understand the CODPNTDR section of vol3 (page 224): Codepoints consist of two values, a 1-hex index into a list of dictionaries and a 3-hex identifier unique within the referenced dictionary. The resolution of codepoints to class descriptors is illustrated in Figure 3-18 (on page 225). For Agent Level 3, the list of dictionaries is fixed, and specific index values are assigned as follows: • C-F: Reserved for product extensions (see EXTENSIONS (on page 381)) • 4-B: Reserved for DDM Architecture • 3: DDM dictionary QDDADLD • 2: DDM dictionary QDDRDBD • 1: DDM dictionary QDDBASD • 0: DDM dictionary QDDPRMD Does that mean I can use codepoints in the range [0xC000-0xFFFF] to product-unique stuff? And use them as instance variables in OPNQRYRM/QRYDTA?
          Hide
          Knut Anders Hatlen added a comment -

          > Does that mean I can use codepoints in the range [0xC000-0xFFFF] to product-unique stuff?

          Yes, that's how I interpret it.

          > And use them as instance variables in OPNQRYRM/QRYDTA?

          I think so. EXTENSIONS on page 381 says:

          > In both cases, the framework of existing DDM classes are used as the basis for extensions to
          > DDM architecture. DDM allows the following "open architecture" enhancements:
          > • Whole new classes of objects (such as libraries or mailboxes) can be defined, either with
          > new commands and replies unique to the class, or with existing DDM commands and
          > replies as appropriate.
          > • Defining new commands for class can enhance the function of DDM classes.
          > • New parameters can be added to existing DDM commands.
          > • New values can be defined as valid for existing DDM parameters.

          Strictly speaking, OPNQRYRM and QRYDTA are replies, not commands. I don't see why we should be allowed to add parameters to existing DDM commands, but not to existing replies. If we want to follow the spec's letter, we may need to invent a new codepoint for the reply messages too, not only for the added instance variables. I may be missing something, though...

          Show
          Knut Anders Hatlen added a comment - > Does that mean I can use codepoints in the range [0xC000-0xFFFF] to product-unique stuff? Yes, that's how I interpret it. > And use them as instance variables in OPNQRYRM/QRYDTA? I think so. EXTENSIONS on page 381 says: > In both cases, the framework of existing DDM classes are used as the basis for extensions to > DDM architecture. DDM allows the following "open architecture" enhancements: > • Whole new classes of objects (such as libraries or mailboxes) can be defined, either with > new commands and replies unique to the class, or with existing DDM commands and > replies as appropriate. > • Defining new commands for class can enhance the function of DDM classes. > • New parameters can be added to existing DDM commands. > • New values can be defined as valid for existing DDM parameters. Strictly speaking, OPNQRYRM and QRYDTA are replies, not commands. I don't see why we should be allowed to add parameters to existing DDM commands, but not to existing replies. If we want to follow the spec's letter, we may need to invent a new codepoint for the reply messages too, not only for the added instance variables. I may be missing something, though...
          Hide
          Dyre Tjeldvoll added a comment -

          I'm attaching a completely reworked
          patch (derby-3192-mark2.v1.diff) based on the new idea of adding
          product-specific code points. All tests pass.

          I plan to update the writeup on the Wiki as well, but here is a
          short summary of the new patch:

          • Adds a new method (getCurrentSchemaName()) to EmbedConnection
            and EngineConnection that the NetworkServer can use to find out
            what the current schema is.
          • Adds functionality to the Database class that keeps track of
            the latest isolation level and schema which have been
            sent to the client, and whether the values have changed
            since the last piggy-backing.
          • Adds three product-specific code points, PBSD, PBSD_ISO and
            PBSD_SCHEMA as well as a method (DRDAConnThread.writePBSD)
            which adds a PBSD containing one or both of PBSD_ISO and PBSD_SCHEMA
            to outgoing messages when piggy-backing is needed.
          • The reply to EXCSQLIMM, EXSQLSTT, OPNQRY and CNTQRY will
            include session data when needed. There is no additional data
            being transmitted when session attributes are unchanged.
          • The corresponding parse methods for the replies on the client
            have been augmented to handle the new code points and invoke a
            callback on the client which updates (syncs) the cached session
            data.
          • am.Connection on the client side is modified to use its cached
            copy of the session attributes, unless they have a
            special "unknown" value which will force a fetch from the
            server (as before).
          • Joining/leaving an XA transaction is handled by dropping the
            cached values (by assigning the special "unknown" value).
          • A new variable and access method for the schema name has been added
            to am.Connection. This is primarily intended for use by the new
            StatementCache
          Show
          Dyre Tjeldvoll added a comment - I'm attaching a completely reworked patch (derby-3192-mark2.v1.diff) based on the new idea of adding product-specific code points. All tests pass. I plan to update the writeup on the Wiki as well, but here is a short summary of the new patch: Adds a new method (getCurrentSchemaName()) to EmbedConnection and EngineConnection that the NetworkServer can use to find out what the current schema is. Adds functionality to the Database class that keeps track of the latest isolation level and schema which have been sent to the client, and whether the values have changed since the last piggy-backing. Adds three product-specific code points, PBSD, PBSD_ISO and PBSD_SCHEMA as well as a method (DRDAConnThread.writePBSD) which adds a PBSD containing one or both of PBSD_ISO and PBSD_SCHEMA to outgoing messages when piggy-backing is needed. The reply to EXCSQLIMM, EXSQLSTT, OPNQRY and CNTQRY will include session data when needed. There is no additional data being transmitted when session attributes are unchanged. The corresponding parse methods for the replies on the client have been augmented to handle the new code points and invoke a callback on the client which updates (syncs) the cached session data. am.Connection on the client side is modified to use its cached copy of the session attributes, unless they have a special "unknown" value which will force a fetch from the server (as before). Joining/leaving an XA transaction is handled by dropping the cached values (by assigning the special "unknown" value). A new variable and access method for the schema name has been added to am.Connection. This is primarily intended for use by the new StatementCache
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v1.diff [ 12375177 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Note that there is a bug in the mark2-v1 patch. The Database.pbsd_ variable is not assigned when a new object is created so session data will be piggybacked every time, not only when they have changed.
          I'll upload a new patch.

          Show
          Dyre Tjeldvoll added a comment - Note that there is a bug in the mark2-v1 patch. The Database.pbsd_ variable is not assigned when a new object is created so session data will be piggybacked every time, not only when they have changed. I'll upload a new patch.
          Hide
          A B added a comment -

          > ... based on the new idea of adding product-specific code points.

          I assume these new code points will be completely hidden from, and ignored by, connections/communication with non-Derby clients, esp. ODBC. Is that correct?

          Show
          A B added a comment - > ... based on the new idea of adding product-specific code points. I assume these new code points will be completely hidden from, and ignored by, connections/communication with non-Derby clients, esp. ODBC. Is that correct?
          Hide
          Dyre Tjeldvoll added a comment -

          AB> I assume these new code points will be completely hidden from, and ignored by, connections/communication with non-Derby clients, esp. ODBC. Is that correct?

          Yes. The intention is that they would only be sent to Derby client drivers version 10.4 and newer. Right now I have the following

          private void writePBSD() throws SQLException, DRDAProtocolException
          {
          if (!appRequester.greaterThanOrEqualTo(10, 4, 0))

          { return; }

          Looking at it again, I guess I should add

          appRequester.getClientType() != AppRequester.DNC_CLIENT

          to the condition as well.

          Show
          Dyre Tjeldvoll added a comment - AB> I assume these new code points will be completely hidden from, and ignored by, connections/communication with non-Derby clients, esp. ODBC. Is that correct? Yes. The intention is that they would only be sent to Derby client drivers version 10.4 and newer. Right now I have the following private void writePBSD() throws SQLException, DRDAProtocolException { if (!appRequester.greaterThanOrEqualTo(10, 4, 0)) { return; } Looking at it again, I guess I should add appRequester.getClientType() != AppRequester.DNC_CLIENT to the condition as well.
          Hide
          A B added a comment -

          > Yes. The intention is that they would only be sent to Derby client drivers version 10.4 and newer.

          Thanks for verifying, Dyre.

          Show
          A B added a comment - > Yes. The intention is that they would only be sent to Derby client drivers version 10.4 and newer. Thanks for verifying, Dyre.
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching a new patch (mark2.v2) which fixes missing assignment to Database.pbsd_ and adds an ASSERT to verify this. Also added a test to ensure that the new code points only get sent to DNC clients.

          Show
          Dyre Tjeldvoll added a comment - Attaching a new patch (mark2.v2) which fixes missing assignment to Database.pbsd_ and adds an ASSERT to verify this. Also added a test to ensure that the new code points only get sent to DNC clients.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v2.diff [ 12375334 ]
          Hide
          Dyre Tjeldvoll added a comment - - edited

          I just noticed that Netbeans had introduced a number of property changes that ended up in the v2 patch. Here is
          a new version without the property changes. Sorry about the noise.

          Show
          Dyre Tjeldvoll added a comment - - edited I just noticed that Netbeans had introduced a number of property changes that ended up in the v2 patch. Here is a new version without the property changes. Sorry about the noise.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v3.diff [ 12375347 ]
          Hide
          Dyre Tjeldvoll added a comment -

          I have updated the writeup at
          http://wiki.apache.org/db-derby/Derby3192Writeup
          so that it correctly describes the mark2.v3 patch.

          Show
          Dyre Tjeldvoll added a comment - I have updated the writeup at http://wiki.apache.org/db-derby/Derby3192Writeup so that it correctly describes the mark2.v3 patch.
          Hide
          Knut Anders Hatlen added a comment -

          I think the approach used in the patch looks fine, though I haven't
          tested it yet. Please see my comments and questions below.

          • DRDAConnThread.writePBSD() checks client type and version. It's
            perhaps cleaner if we put the check in a method in AppRequester (for
            instance, supportsPiggybackedSessionState).
          • Is there any reason why the definition of the PiggyBackedSessionData
            class is appended to Database.java rather than put in a separate
            file? The class is not private to Database and keeping it there
            might make it harder to find it.
          • I think DRDAConnThread.trace() is normally only called when
            SanityManager.DEBUG is true, but writePBSD() sometimes call it
            regardless of the sanity setting.
          • Could "catch (Throwable t) { t.printStackTrace(); }

            " be removed from
            DRDAConnThread.writePBSD()?

          • BrokeredConnection.getCurrentSchemaName() uses a mix of tabs and
            spaces.
          • EmbedConnection.getCurrentSchemaName() is declared as throws
            SQLException, but I don't think any of the methods it calls throws
            SQLException.
          • The debug code at the end of DRDAConnThread.processCommands()
            allocates and populates a Hashtable each time it is executed,
            whereas it is only needed if an error has occurred. Perhaps it could
            be allocated inside the if block and the catch block instead? Not a
            big issue, though, since it's only in debug code. (This makes me
            wish CodePointNameTable had static fields and methods, but that's
            another patch in another JIRA issue...)
          • NetStatementReply.parsePBSD() and
            Connection/NetConnection.completePiggyBackSessionData() use an
            Object array to represent the session state, and use the magic
            numbers 0 and 1 to access the isolation level and the schema,
            respectively. At a minimum, I think we should document in
            Connection.completePiggyBackSessionData()'s javadoc comment what's
            stored in each element of the array. It does however sound easier to
            understand and maintain the code if we create a separate class to
            hold this information. Or perhaps even simpler:
            completePiggyBackSessionData() could take two arguments instead of a
            two element array.
          • In Connection.getTransactionIsolation(), I think the scope of the
            isolation variable could be narrowed down a bit. I believe it could
            be declared where it's assigned the first time, and the return
            statement can be moved inside the try block.
          Show
          Knut Anders Hatlen added a comment - I think the approach used in the patch looks fine, though I haven't tested it yet. Please see my comments and questions below. DRDAConnThread.writePBSD() checks client type and version. It's perhaps cleaner if we put the check in a method in AppRequester (for instance, supportsPiggybackedSessionState). Is there any reason why the definition of the PiggyBackedSessionData class is appended to Database.java rather than put in a separate file? The class is not private to Database and keeping it there might make it harder to find it. I think DRDAConnThread.trace() is normally only called when SanityManager.DEBUG is true, but writePBSD() sometimes call it regardless of the sanity setting. Could "catch (Throwable t) { t.printStackTrace(); } " be removed from DRDAConnThread.writePBSD()? BrokeredConnection.getCurrentSchemaName() uses a mix of tabs and spaces. EmbedConnection.getCurrentSchemaName() is declared as throws SQLException, but I don't think any of the methods it calls throws SQLException. The debug code at the end of DRDAConnThread.processCommands() allocates and populates a Hashtable each time it is executed, whereas it is only needed if an error has occurred. Perhaps it could be allocated inside the if block and the catch block instead? Not a big issue, though, since it's only in debug code. (This makes me wish CodePointNameTable had static fields and methods, but that's another patch in another JIRA issue...) NetStatementReply.parsePBSD() and Connection/NetConnection.completePiggyBackSessionData() use an Object array to represent the session state, and use the magic numbers 0 and 1 to access the isolation level and the schema, respectively. At a minimum, I think we should document in Connection.completePiggyBackSessionData()'s javadoc comment what's stored in each element of the array. It does however sound easier to understand and maintain the code if we create a separate class to hold this information. Or perhaps even simpler: completePiggyBackSessionData() could take two arguments instead of a two element array. In Connection.getTransactionIsolation(), I think the scope of the isolation variable could be narrowed down a bit. I believe it could be declared where it's assigned the first time, and the return statement can be moved inside the try block.
          Hide
          Dyre Tjeldvoll added a comment -

          Hi Knut,

          thank you for your through review. Basically I think all of your comment are valid, except for

          • EmbedConnection.getCurrentSchemaName() is declared as throws
            SQLException, but I don't think any of the methods it calls throws
            SQLException.

          What you're saying here is true, but the problem is that getCurrentSchemaName() is declared in the EngineConnection interface which is also implemented by BrokeredConnection. And in order to follow the BrokeredConnection pattern of checking that the actual connection isn't closed before forwarding, you have to either propagate the SQLException (as the other methods do) or catch it and do something clever. I guess a comment would be in order here...

          Show
          Dyre Tjeldvoll added a comment - Hi Knut, thank you for your through review. Basically I think all of your comment are valid, except for EmbedConnection.getCurrentSchemaName() is declared as throws SQLException, but I don't think any of the methods it calls throws SQLException. What you're saying here is true, but the problem is that getCurrentSchemaName() is declared in the EngineConnection interface which is also implemented by BrokeredConnection. And in order to follow the BrokeredConnection pattern of checking that the actual connection isn't closed before forwarding, you have to either propagate the SQLException (as the other methods do) or catch it and do something clever. I guess a comment would be in order here...
          Hide
          Knut Anders Hatlen added a comment -

          I don't think the implementation in EmbedConnection needs to throw SQLException, even if the interface says it can. See for instance getLOBMapping() which is declared as "throws SQLException" in EngineConnection and BrokeredConnection, and doesn't have any throws clause in EmbedConnection.

          Show
          Knut Anders Hatlen added a comment - I don't think the implementation in EmbedConnection needs to throw SQLException, even if the interface says it can. See for instance getLOBMapping() which is declared as "throws SQLException" in EngineConnection and BrokeredConnection, and doesn't have any throws clause in EmbedConnection.
          Hide
          Dyre Tjeldvoll added a comment -

          Ok, I just assumed that since the exception spec was part of the signature it had to be identical.

          I have added a supportsSessionDataCaching predicate to AppRequester as you suggest (changed the wording slightly to match that used elsewhere).
          I see that there is precedence for doing it this way, but I don't like it. As long as getClientType() and greaterThanOrEqualTo() are accessible, the interface is complete and minimal as it should be. Adding convenience methods on top of that bloats the interface IMHO. (It also needlessly grants the convenience methods access to the internals of AppRequester)

          I tried moving the call to the CodePointTableName constructor into the try-block as you suggest, but then I get a compilation error because the code point string is referenced in the catch-block... I guess I could always make a static final instance in DRDAConnThread, and have a static initializer which assigns null unless it is a DEBUG build, or something. But then you introduce the risk of dereferencing that pointer in non-debug mode...

          Show
          Dyre Tjeldvoll added a comment - Ok, I just assumed that since the exception spec was part of the signature it had to be identical. I have added a supportsSessionDataCaching predicate to AppRequester as you suggest (changed the wording slightly to match that used elsewhere). I see that there is precedence for doing it this way, but I don't like it. As long as getClientType() and greaterThanOrEqualTo() are accessible, the interface is complete and minimal as it should be. Adding convenience methods on top of that bloats the interface IMHO. (It also needlessly grants the convenience methods access to the internals of AppRequester) I tried moving the call to the CodePointTableName constructor into the try-block as you suggest, but then I get a compilation error because the code point string is referenced in the catch-block... I guess I could always make a static final instance in DRDAConnThread, and have a static initializer which assigns null unless it is a DEBUG build, or something. But then you introduce the risk of dereferencing that pointer in non-debug mode...
          Hide
          Knut Anders Hatlen added a comment -

          I think I agree with your comments about the supports methods. I still think it's a good idea to have the check in a separate method, but perhaps a private method in DRDAConnThread would do?

          Regarding CodePointNameTable, I think the simplest way is to allocate a new one in the catch block. Having a static field sounds a bit too complex, given that this is just debug code. I'd say just leave it as it is for now. We can always rewrite CodePointNameTable.lookup() as a static method later, if it becomes a problem in the sane builds.

          Show
          Knut Anders Hatlen added a comment - I think I agree with your comments about the supports methods. I still think it's a good idea to have the check in a separate method, but perhaps a private method in DRDAConnThread would do? Regarding CodePointNameTable, I think the simplest way is to allocate a new one in the catch block. Having a static field sounds a bit too complex, given that this is just debug code. I'd say just leave it as it is for now. We can always rewrite CodePointNameTable.lookup() as a static method later, if it becomes a problem in the sane builds.
          Hide
          Dyre Tjeldvoll added a comment -

          Uploading mark2.v4 to address the comments.

          Show
          Dyre Tjeldvoll added a comment - Uploading mark2.v4 to address the comments.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v4.diff [ 12375939 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Yet another version, hopefully without trailing white space this time...

          Show
          Dyre Tjeldvoll added a comment - Yet another version, hopefully without trailing white space this time...
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v5.diff [ 12376018 ]
          Hide
          Kristian Waagan added a comment -

          I had a quick look at the patch (mark2.v5) to see if I can easily start using it as part of the statement pooling feature, and I believe I can.

          I did notice one thing in am.Connection.getCurrentSchemaName:
          + System.err.println("*** Need query to get current schema ***");

          Is it supposed to be there?

          Show
          Kristian Waagan added a comment - I had a quick look at the patch (mark2.v5) to see if I can easily start using it as part of the statement pooling feature, and I believe I can. I did notice one thing in am.Connection.getCurrentSchemaName: + System.err.println("*** Need query to get current schema ***"); Is it supposed to be there?
          Hide
          Dyre Tjeldvoll added a comment -

          Kristian> Is it supposed to be there?

          Yes and no. No, it should probably not be there permanently, but I wanted to get some kind of warning if this starts to happen more frequently than we expect it to. Otherwise, if a bug kept setting the cached schema name to null, then we would not see it (other than as a performance degradation). Maybe that line should have been put into some kind of client-side tracing system...

          Show
          Dyre Tjeldvoll added a comment - Kristian> Is it supposed to be there? Yes and no. No, it should probably not be there permanently, but I wanted to get some kind of warning if this starts to happen more frequently than we expect it to. Otherwise, if a bug kept setting the cached schema name to null, then we would not see it (other than as a performance degradation). Maybe that line should have been put into some kind of client-side tracing system...
          Hide
          Dyre Tjeldvoll added a comment -

          Here is a version of the patch where the stderr println has been replaced by agent tracing.

          Show
          Dyre Tjeldvoll added a comment - Here is a version of the patch where the stderr println has been replaced by agent tracing.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v6.diff [ 12376209 ]
          Hide
          Knut Anders Hatlen added a comment -

          Thanks for the updated patch, Dyre. I think it looks good and ready for commit now. I still plan to test it more thoroughly, but that doesn't mean you should hold the commit. If I find any issues, they can be handled in follow-ups.

          One question about the SYNCCTL messages: Would it make sense to write PBSD in the reply even though the client ignores it for 10.4? Could that make it easier to implement the session state caching for XA later? I was thinking then it perhaps would be a pure client-side fix to enable it for XA, and the client didn't need special handling of yet another version (that is, you wouldn't need both serverSupportsSessionDataCaching() and serverSupportsSessionDataCachingForXA() on the client). I'm not sure whether this is possible and/or desirable, but I thought I'd mention it.

          Show
          Knut Anders Hatlen added a comment - Thanks for the updated patch, Dyre. I think it looks good and ready for commit now. I still plan to test it more thoroughly, but that doesn't mean you should hold the commit. If I find any issues, they can be handled in follow-ups. One question about the SYNCCTL messages: Would it make sense to write PBSD in the reply even though the client ignores it for 10.4? Could that make it easier to implement the session state caching for XA later? I was thinking then it perhaps would be a pure client-side fix to enable it for XA, and the client didn't need special handling of yet another version (that is, you wouldn't need both serverSupportsSessionDataCaching() and serverSupportsSessionDataCachingForXA() on the client). I'm not sure whether this is possible and/or desirable, but I thought I'd mention it.
          Hide
          Knut Anders Hatlen added a comment -

          One more thing: Shouldn't am.Connection.getCurrentSchemaName() close the Statement it creates?

          Show
          Knut Anders Hatlen added a comment - One more thing: Shouldn't am.Connection.getCurrentSchemaName() close the Statement it creates?
          Hide
          Knut Anders Hatlen added a comment -

          And perhaps PiggyBackedSessionData.conn_ should be declared final just to spell out that a new PBSD object must be created when the connection changes.

          Show
          Knut Anders Hatlen added a comment - And perhaps PiggyBackedSessionData.conn_ should be declared final just to spell out that a new PBSD object must be created when the connection changes.
          Hide
          Dyre Tjeldvoll added a comment -

          Hi Knut, thanks for the comments. I have added closing of the statement, and made conn_ final in my sandbox.

          I see your point about avoiding more compatibility tests by adding piggy-backing to the SYNCCTL reply right away. I have not looked at the code in detail yet, but I suspect that you cannot ignore the PBSD without adding the parsing necessary to confirm that it actually is a PBSD. And then most of the job is already done, because then all that needs to be done is to call readPBSD(). I'll try experiment with this to see if it works...

          Show
          Dyre Tjeldvoll added a comment - Hi Knut, thanks for the comments. I have added closing of the statement, and made conn_ final in my sandbox. I see your point about avoiding more compatibility tests by adding piggy-backing to the SYNCCTL reply right away. I have not looked at the code in detail yet, but I suspect that you cannot ignore the PBSD without adding the parsing necessary to confirm that it actually is a PBSD. And then most of the job is already done, because then all that needs to be done is to call readPBSD(). I'll try experiment with this to see if it works...
          Hide
          Knut Anders Hatlen added a comment -

          Thanks Dyre. But don't let the experiment stop you from committing the patch as it is. The main issue is not whether or not it is part of the first check-in, but whether or not it is part of 10.4. And even if it doesn't make it for 10.4, it's not a very big deal.

          Show
          Knut Anders Hatlen added a comment - Thanks Dyre. But don't let the experiment stop you from committing the patch as it is. The main issue is not whether or not it is part of the first check-in, but whether or not it is part of 10.4. And even if it doesn't make it for 10.4, it's not a very big deal.
          Hide
          Dyre Tjeldvoll added a comment -

          Sending PBSD with the SYNCCTL reply seemed to work, after some trouble-shooting. Initially I was fooled by the fact that both NetConnection and XANetConnection implementes parseSYNCCTLreply(). My first attempt failed because I had only added PBSD handling to NetConnection.
          Seems safer to handle PBSD in both classes, although I would expect that only NetXAConnection actually needs to.

          I plan to post another patch when I've removed yet another set of debug printouts...

          Show
          Dyre Tjeldvoll added a comment - Sending PBSD with the SYNCCTL reply seemed to work, after some trouble-shooting. Initially I was fooled by the fact that both NetConnection and XANetConnection implementes parseSYNCCTLreply(). My first attempt failed because I had only added PBSD handling to NetConnection. Seems safer to handle PBSD in both classes, although I would expect that only NetXAConnection actually needs to. I plan to post another patch when I've removed yet another set of debug printouts...
          Hide
          Dyre Tjeldvoll added a comment -

          OK, here is v7 which sends PBSD in SYNCCTLreply and uses the extra piggy-backing to avoid clearing the cached attributes in every XA state change. All tests pass.

          I plan to update the writeup to reflect the changes.

          I intend to commit tomorrow unless there are showstopper-comments.

          Show
          Dyre Tjeldvoll added a comment - OK, here is v7 which sends PBSD in SYNCCTLreply and uses the extra piggy-backing to avoid clearing the cached attributes in every XA state change. All tests pass. I plan to update the writeup to reflect the changes. I intend to commit tomorrow unless there are showstopper-comments.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v7.diff [ 12376485 ]
          Hide
          Dyre Tjeldvoll added a comment -

          v8 moves parsePBSD() from Reply to NetConnection where it logically belongs, and also calls parseCommonError() if an unexpected sub code point is found in parsePBSD(). Please disregard the v7 version.

          Show
          Dyre Tjeldvoll added a comment - v8 moves parsePBSD() from Reply to NetConnection where it logically belongs, and also calls parseCommonError() if an unexpected sub code point is found in parsePBSD(). Please disregard the v7 version.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-mark2.v8.diff [ 12376489 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Committed revision 631593

          Show
          Dyre Tjeldvoll added a comment - Committed revision 631593
          Hide
          Dyre Tjeldvoll added a comment -

          I have updated the writeup so that it matches the committed patch. When doing that it occurred to me that I should probably remove the special handling of SYNCCTL in my sanity ASSERT at the bottom of the processCommands-switch. I'll upload a follow-up patch when I've tested it.

          Show
          Dyre Tjeldvoll added a comment - I have updated the writeup so that it matches the committed patch. When doing that it occurred to me that I should probably remove the special handling of SYNCCTL in my sanity ASSERT at the bottom of the processCommands-switch. I'll upload a follow-up patch when I've tested it.
          Hide
          Dyre Tjeldvoll added a comment -

          Attaching fup.v1 which removes the special treatment of SYNCCTL in the sanity check. All tests still pass.

          Show
          Dyre Tjeldvoll added a comment - Attaching fup.v1 which removes the special treatment of SYNCCTL in the sanity check. All tests still pass.
          Dyre Tjeldvoll made changes -
          Attachment derby-3192-fup.v1.diff [ 12376735 ]
          Hide
          Dyre Tjeldvoll added a comment -

          Committed derby-3192-fup.v1.diff with revision 633011.

          Show
          Dyre Tjeldvoll added a comment - Committed derby-3192-fup.v1.diff with revision 633011.
          Hide
          Dyre Tjeldvoll added a comment -

          I'm resolving this as I have not seen any regressions in the testing so far.

          Show
          Dyre Tjeldvoll added a comment - I'm resolving this as I have not seen any regressions in the testing so far.
          Dyre Tjeldvoll made changes -
          Status In Progress [ 3 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Dyre Tjeldvoll made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Dag H. Wanvik made changes -
          Component/s Performance [ 11709 ]
          Dag H. Wanvik made changes -
          Derby Categories [Performance]
          Kathey Marsden made changes -
          Fix Version/s 10.4.1.3 [ 12313111 ]
          Gavin made changes -
          Link This issue depends on DERBY-3198 [ DERBY-3198 ]
          Gavin made changes -
          Link This issue depends upon DERBY-3198 [ DERBY-3198 ]
          Gavin made changes -
          Link This issue blocks DERBY-3313 [ DERBY-3313 ]
          Gavin made changes -
          Link This issue is depended upon by DERBY-3313 [ DERBY-3313 ]
          Gavin made changes -
          Workflow jira [ 12416757 ] Default workflow, editable Closed status [ 12802321 ]

            People

            • Assignee:
              Dyre Tjeldvoll
              Reporter:
              Dyre Tjeldvoll
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development