Derby
  1. Derby
  2. DERBY-775

Network client: Add support for scrollable, updatable, insensitive result sets

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 10.2.1.6
    • Component/s: JDBC, Network Client
    • Labels:
      None
    • Issue & fix info:
      Release Note Needed

      Description

      This is a part of the DERBY-690 effort.

      1. 775-writeup.txt
        7 kB
        Dag H. Wanvik
      2. derby-775-1.diff
        94 kB
        Dag H. Wanvik
      3. derby-775-1.stat
        3 kB
        Dag H. Wanvik
      4. derby-775-2.diff
        96 kB
        Dag H. Wanvik
      5. derby-775-2.stat
        3 kB
        Dag H. Wanvik
      6. derby-775-3.diff
        104 kB
        Dag H. Wanvik
      7. derby-775-3.stat
        3 kB
        Dag H. Wanvik
      8. derby-775-4.diff
        134 kB
        Dag H. Wanvik
      9. derby-775-4.stat
        3 kB
        Dag H. Wanvik
      10. derby-775-5.diff
        138 kB
        Andreas Korneliussen
      11. derby-775-5.stat
        3 kB
        Andreas Korneliussen
      12. derby-775-6.diff
        158 kB
        Dag H. Wanvik
      13. derby-775-6.stat
        3 kB
        Dag H. Wanvik
      14. derby-775-7.diff
        158 kB
        Dag H. Wanvik
      15. derby-775-7.stat
        3 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          Posting this in the hope that some of you DRDA experts can comment on
          it

          I have been trying to figure out how to map JDBC scrollable,
          updatable, insensitive result sets (SUR for short) onto DRDA and have
          three questions (below). I am assuming the semantics described in the
          attachedment to DERBY-690.

          Some background:

          DRDA has no concept of JDBC result sets, so the solution used in Derby
          is to map JDBC result sets onto the DRDA cursor mechanism. Appendix B
          in "DRDA, Version 3, Volume 1: Distributed Relational Database
          Architecture (DRDA)" gives an overview of "Scrollable Cursors" (p
          655).

          a) Sensitivity

          I first considered to map SUR to "insensitive scrollable cursors", but
          DRDA states that these are read only (B.2.2). It seems the "sensitive
          static cursors" better matches the semantics we want: For this
          category of cursors, the size of the result set ("result table" in
          DRDA parlance) as well as the ordering of rows are fixed and this
          cursor is updatable, cf. quote on page 656:

          "The cursor is always immediately sensitive to changes made using
          the cursor (that is, positioned updates and deletes using the
          same cursor).

          The size of the result table does not grow after the cursor is
          opened and the rows are materialized. The order of the rows is
          established as the result table is materialized."

          Combined with the proper fetch, i.e. fetch INSENSITIVE (p 658) on
          CNTQRY (i.e. we are INSENSITIVE to changes made by others), the
          semantics match those of JDBC's insensitive result sets as we have
          specified them :

          ownDeletesAreVisible(TYPE_SCROLL_INSENSITIVE) -> true
          ownInsertsAreVisible(TYPE_SCROLL_INSENSITIVE) -> false
          ownUpdatesAreVisible(TYPE_SCROLL_INSENSITIVE) -> true

          othersDeletesAreVisible(TYPE_SCROLL_INSENSITIVE) -> false
          othersInsertsAreVisible(TYPE_SCROLL_INSENSITIVE) -> false
          othersUpdatesAreVisible(TYPE_SCROLL_INSENSITIVE) -> false

          Question 1: Can anyone see a problem with mapping JDBC "updatable,
          insensitive" onto DRDA "sensitive static"? Would this be violating
          (the spirit of) the DRDA in any way?

          b) Detectability

          In the spec, we required that we be able to detect deletes and updates
          to the rows in the result set:

          deletesAreDetected(TYPE_SCROLL_INSENSITIVE) -> true
          updatesAreDetected(TYPE_SCROLL_INSENSITIVE) -> true

          (Since inserts are not visible, they can not be detectable, either).
          DRDA supports detection of holes in the following manner (quote,
          P. 656):

          "To present a static size and static ordering for the result table, the
          relational database may return a hole to the application that fetches
          an updated or deleted row in the result table. A hole in the result
          table occurs when there is a difference between the result table and
          the underlying base table. No data can be fetched from a hole, and the
          hole is manifested in the QRYDTA as a row consisting of a non-null
          SQLCARD and a null data group.

          When the current value of a row no longer satisfies the
          select-statement or statement-name, that row is visible in the cursor
          as an update hole , where the SQLCARD has a warning SQLSTATE of 02502.

          When a row of the result table is deleted from the underlying base
          table, the row is visible in the cursor as a delete hole , where the
          SQLCARD has a warning SQLSTATE of 02502."

          For deletes, the "delete hole" is exactly what we need to support
          ResultSet#rowDeleted().

          For updates, is is not exactly what we want, since we do not intend to
          requalify a row after it has been updated (thereby possibly making it
          an "update hole"). On the other hand, when we update a row and let it
          remain in the result table, DRDA offers no means of conveying that the
          row has been changed in the sense of JDBC ResultSet#rowUpdated(), as
          far as I can tell.

          Question 2: Is there some way we can detect the latter without
          violating the protocol? One could imagine signalling this using
          another warning SQLSTATE. Would this be an acceptable tweaking of the
          DRDA?

          c) Query protocol

          For scrollable result sets, Derby uses the "Limited Block Query
          Protocol". This is allowable also for "static sensitive", so I assume
          we can use that also for SUR in the way it is presently used for
          scrollable, read-only result sets.

          d) Rowset cursors (p 667)

          "Rowset cursors" seem not to be in use by the server, but the client
          has code for handling it. Not sure at this if this code needs updating
          for SUR..

          Question 3: Is it a requirement that the client be able to handle
          row sets to be DRDA compliant? (We know our server doesn't use it for
          now....)

          d) Equality of result set modification and positioned delete/update.

          The same cursor is being used, and the server (and our proposed
          implementation of SUR) is agnostic to whether an updateRow/deleteRow
          or a positioned update/delete is being invoked by the network client
          driver, the only difference being the autocommit semantics, which is
          handled by the driver.

          Show
          Dag H. Wanvik added a comment - Posting this in the hope that some of you DRDA experts can comment on it I have been trying to figure out how to map JDBC scrollable, updatable, insensitive result sets (SUR for short) onto DRDA and have three questions (below). I am assuming the semantics described in the attachedment to DERBY-690 . Some background: DRDA has no concept of JDBC result sets, so the solution used in Derby is to map JDBC result sets onto the DRDA cursor mechanism. Appendix B in "DRDA, Version 3, Volume 1: Distributed Relational Database Architecture (DRDA)" gives an overview of "Scrollable Cursors" (p 655). a) Sensitivity I first considered to map SUR to "insensitive scrollable cursors", but DRDA states that these are read only (B.2.2). It seems the "sensitive static cursors" better matches the semantics we want: For this category of cursors, the size of the result set ("result table" in DRDA parlance) as well as the ordering of rows are fixed and this cursor is updatable, cf. quote on page 656: "The cursor is always immediately sensitive to changes made using the cursor (that is, positioned updates and deletes using the same cursor). The size of the result table does not grow after the cursor is opened and the rows are materialized. The order of the rows is established as the result table is materialized." Combined with the proper fetch, i.e. fetch INSENSITIVE (p 658) on CNTQRY (i.e. we are INSENSITIVE to changes made by others), the semantics match those of JDBC's insensitive result sets as we have specified them : ownDeletesAreVisible(TYPE_SCROLL_INSENSITIVE) -> true ownInsertsAreVisible(TYPE_SCROLL_INSENSITIVE) -> false ownUpdatesAreVisible(TYPE_SCROLL_INSENSITIVE) -> true othersDeletesAreVisible(TYPE_SCROLL_INSENSITIVE) -> false othersInsertsAreVisible(TYPE_SCROLL_INSENSITIVE) -> false othersUpdatesAreVisible(TYPE_SCROLL_INSENSITIVE) -> false Question 1 : Can anyone see a problem with mapping JDBC "updatable, insensitive" onto DRDA "sensitive static"? Would this be violating (the spirit of) the DRDA in any way? b) Detectability In the spec, we required that we be able to detect deletes and updates to the rows in the result set: deletesAreDetected(TYPE_SCROLL_INSENSITIVE) -> true updatesAreDetected(TYPE_SCROLL_INSENSITIVE) -> true (Since inserts are not visible, they can not be detectable, either). DRDA supports detection of holes in the following manner (quote, P. 656): "To present a static size and static ordering for the result table, the relational database may return a hole to the application that fetches an updated or deleted row in the result table. A hole in the result table occurs when there is a difference between the result table and the underlying base table. No data can be fetched from a hole, and the hole is manifested in the QRYDTA as a row consisting of a non-null SQLCARD and a null data group. When the current value of a row no longer satisfies the select-statement or statement-name, that row is visible in the cursor as an update hole , where the SQLCARD has a warning SQLSTATE of 02502. When a row of the result table is deleted from the underlying base table, the row is visible in the cursor as a delete hole , where the SQLCARD has a warning SQLSTATE of 02502." For deletes, the "delete hole" is exactly what we need to support ResultSet#rowDeleted(). For updates, is is not exactly what we want, since we do not intend to requalify a row after it has been updated (thereby possibly making it an "update hole"). On the other hand, when we update a row and let it remain in the result table, DRDA offers no means of conveying that the row has been changed in the sense of JDBC ResultSet#rowUpdated(), as far as I can tell. Question 2 : Is there some way we can detect the latter without violating the protocol? One could imagine signalling this using another warning SQLSTATE. Would this be an acceptable tweaking of the DRDA? c) Query protocol For scrollable result sets, Derby uses the "Limited Block Query Protocol". This is allowable also for "static sensitive", so I assume we can use that also for SUR in the way it is presently used for scrollable, read-only result sets. d) Rowset cursors (p 667) "Rowset cursors" seem not to be in use by the server, but the client has code for handling it. Not sure at this if this code needs updating for SUR.. Question 3 : Is it a requirement that the client be able to handle row sets to be DRDA compliant? (We know our server doesn't use it for now....) d) Equality of result set modification and positioned delete/update. The same cursor is being used, and the server (and our proposed implementation of SUR) is agnostic to whether an updateRow/deleteRow or a positioned update/delete is being invoked by the network client driver, the only difference being the autocommit semantics, which is handled by the driver.
          Hide
          Dag H. Wanvik added a comment -

          Here is a patch and write-up for the client-server version of SUR. Tested
          with derbyall on Solaris 10/x86, Sun JVM 1.4.2 with these errors:

          NSinSameJVM.java, DerbyNetNewServer.java, sysinfo.java, sysinfo_withproperties.java
          on both DerbyNet and DerbyNetClient frameworks, believed to be non-related to this
          patch.

          I would appreciate if someone can start looking at it; it is much smaller than the
          patch for embedded SUR.

          Show
          Dag H. Wanvik added a comment - Here is a patch and write-up for the client-server version of SUR. Tested with derbyall on Solaris 10/x86, Sun JVM 1.4.2 with these errors: NSinSameJVM.java, DerbyNetNewServer.java, sysinfo.java, sysinfo_withproperties.java on both DerbyNet and DerbyNetClient frameworks, believed to be non-related to this patch. I would appreciate if someone can start looking at it; it is much smaller than the patch for embedded SUR.
          Hide
          Bryan Pendleton added a comment -

          Thanks for the writeup file! That is very helpful.

          The patch applies cleanly for me, but "ant all" gives:

          compilet1:
          [javac] Compiling 27 source files to /home/bpendleton/src/derby/commit/trunk/classes
          [javac] /home/bpendleton/src/derby/commit/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SURTest.java:59: cannot resolve symbol
          [javac] symbol : variable QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET
          [javac] location: class org.apache.derbyTesting.functionTests.tests.jdbcapi.SURTest
          [javac] assertWarning(warn, QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET);
          [javac] ^
          [javac] /home/bpendleton/src/derby/commit/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SURTest.java:83: cannot resolve symbol
          [javac] symbol : variable QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET
          [javac] location: class org.apache.derbyTesting.functionTests.tests.jdbcapi.SURTest
          [javac] assertWarning(warn, QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET);
          [javac] ^
          [javac] 2 errors
          [javac] 3 warnings

          Any ideas what might be wrong?

          Show
          Bryan Pendleton added a comment - Thanks for the writeup file! That is very helpful. The patch applies cleanly for me, but "ant all" gives: compilet1: [javac] Compiling 27 source files to /home/bpendleton/src/derby/commit/trunk/classes [javac] /home/bpendleton/src/derby/commit/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SURTest.java:59: cannot resolve symbol [javac] symbol : variable QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET [javac] location: class org.apache.derbyTesting.functionTests.tests.jdbcapi.SURTest [javac] assertWarning(warn, QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET); [javac] ^ [javac] /home/bpendleton/src/derby/commit/trunk/java/testing/org/apache/derbyTesting/functionTests/tests/jdbcapi/SURTest.java:83: cannot resolve symbol [javac] symbol : variable QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET [javac] location: class org.apache.derbyTesting.functionTests.tests.jdbcapi.SURTest [javac] assertWarning(warn, QUERY_NOT_QUALIFIED_FOR_UPDATABLE_RESULTSET); [javac] ^ [javac] 2 errors [javac] 3 warnings Any ideas what might be wrong?
          Hide
          Dag H. Wanvik added a comment -

          Note: I did not update the 1.6 canons, as they seemed not up-to-date
          before. I could look into that.

          I also intend to run the upgrade test before asking for a commit (see also
          upgrade comments in the attached write-up).

          Show
          Dag H. Wanvik added a comment - Note: I did not update the 1.6 canons, as they seemed not up-to-date before. I could look into that. I also intend to run the upgrade test before asking for a commit (see also upgrade comments in the attached write-up).
          Hide
          Dag H. Wanvik added a comment -

          The merge conflict that Bryan saw has been resolved. The conflicting
          patch contained a new test which exposed a difference in SUR behavior
          in the client relative to embedded. This has been fixed: After a
          commit, with holdability, the client, in contrast with embedded,
          automatically did a repositioning. This has been changed so both
          drivers require the app to reposition the result set after a commit.

          "derbyall" has been run again with no unexpected failures.

          Show
          Dag H. Wanvik added a comment - The merge conflict that Bryan saw has been resolved. The conflicting patch contained a new test which exposed a difference in SUR behavior in the client relative to embedded. This has been fixed: After a commit, with holdability, the client, in contrast with embedded, automatically did a repositioning. This has been changed so both drivers require the app to reposition the result set after a commit. "derbyall" has been run again with no unexpected failures.
          Hide
          Øystein Grøvlen added a comment -

          So far I have reviewed all code except test changes. The patch looks
          good. I have a few comments/questions:

          1. metadata_net.properties:

          • Is the format and constant values used here documented anywhere?

          2. DRDAConnThread

          • parseSQLATTR(): Variable insensitive can be removed entirely. It
            serves no purpose anymore.

          3. NetResultSet/NetResultSet40

          • Parameters of constructor has comments with possible values. This
            needs to be updated to include QRYSNSSTC.

          4. NetCursor

          • The writeup says the purpose is to pop warnings, but the code seems
            to only handle a single warning.
          • Why do you have to delay the call to setIsUpdataDeleteHole until
            later and not do it when testing for warnings?

          5. NetStatementReply

          • Call to ClientJDBCObjectFactory.newNetResultSet() lists possible
            values. This needs to be updated to include QRYSNSSTC.

          6. Statement

          • Why have you made warnings_ protected? It seems like it is package
            private that is needed. Why not keep it private and supply a
            get-method?

          7. ResultSet

          • Why have you made suggestedFetchSize_ public? It seems like it
            could be protected.
          • Comments for fetchSize_ and suggestedFetchSize_ are a bit brief.
            (And if they are to be public, you should supply javadoc).
          • relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for
            all result sets?
          • relativex(): It seems like the code to call getAbsoluteResultSet is
            duplicated. I do not think the second call will ever be executed.
          • rowUpdated(): Why not always call getIsRowUpdated? I would
            think it was valid for all result sets.
          • rowUpdated()/rowDeleted(): Thss methods are a bit asymmetric.
            rowUpdated() calls a get-method while rowDeleted accesses
            isUpdateDeleteHole_ directly.
          • updateRow(): I tried to browse the spec and the javadoc and I did
            not find anything that said that there is a difference
            between forward only cursors and scrollable cursors with respect to
            validity of the current position after and update. Where is this
            stated?
          • checkForUpdatableResultSet(): What is the difference between this
            new method and the existing checkUpdatableCursor()? It seems better
            to change the existing than to make a new method.
          Show
          Øystein Grøvlen added a comment - So far I have reviewed all code except test changes. The patch looks good. I have a few comments/questions: 1. metadata_net.properties: Is the format and constant values used here documented anywhere? 2. DRDAConnThread parseSQLATTR(): Variable insensitive can be removed entirely. It serves no purpose anymore. 3. NetResultSet/NetResultSet40 Parameters of constructor has comments with possible values. This needs to be updated to include QRYSNSSTC. 4. NetCursor The writeup says the purpose is to pop warnings, but the code seems to only handle a single warning. Why do you have to delay the call to setIsUpdataDeleteHole until later and not do it when testing for warnings? 5. NetStatementReply Call to ClientJDBCObjectFactory.newNetResultSet() lists possible values. This needs to be updated to include QRYSNSSTC. 6. Statement Why have you made warnings_ protected? It seems like it is package private that is needed. Why not keep it private and supply a get-method? 7. ResultSet Why have you made suggestedFetchSize_ public? It seems like it could be protected. Comments for fetchSize_ and suggestedFetchSize_ are a bit brief. (And if they are to be public, you should supply javadoc). relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for all result sets? relativex(): It seems like the code to call getAbsoluteResultSet is duplicated. I do not think the second call will ever be executed. rowUpdated(): Why not always call getIsRowUpdated? I would think it was valid for all result sets. rowUpdated()/rowDeleted(): Thss methods are a bit asymmetric. rowUpdated() calls a get-method while rowDeleted accesses isUpdateDeleteHole_ directly. updateRow(): I tried to browse the spec and the javadoc and I did not find anything that said that there is a difference between forward only cursors and scrollable cursors with respect to validity of the current position after and update. Where is this stated? checkForUpdatableResultSet(): What is the difference between this new method and the existing checkUpdatableCursor()? It seems better to change the existing than to make a new method.
          Hide
          Dag H. Wanvik added a comment -

          Here are answers to Øysteins comments and questions.

          A new version of the patch (#3) reflecting those answers has been
          uploaded. In this version, some too long lines added or changed have
          also been normalized (<= 80 chars wide). I also fixed added or changed
          lines to comply with surrounding whitespace styles (tab or not).

          I ran derbyall using Solaris 10/x86 Sun JRE 1.4.2 with no untoward
          results.

          >>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote:
          Øystein> 1. metadata_net.properties:
          Øystein>
          Øystein> - Is the format and constant values used here documented anywhere?

          As part of DERBY-965, I added some comments explaining the syntax of
          the fields I touched, please see the file from line 197 onwards. I
          am not aware of any other documentation.

          Øystein> 2. DRDAConnThread
          Øystein>
          Øystein> - parseSQLATTR(): Variable insensitive can be removed entirely. It
          Øystein> serves no purpose anymore.

          Agreed, removed.

          Øystein>
          Øystein> 3. NetResultSet/NetResultSet40
          Øystein>
          Øystein> - Parameters of constructor has comments with possible values. This
          Øystein> needs to be updated to include QRYSNSSTC.

          Agreed, done.

          Øystein> 4. NetCursor
          Øystein>
          Øystein> - The writeup says the purpose is to pop warnings, but the code seems
          Øystein> to only handle a single warning.

          Currently, chained warnings are not being sent over DRDA. I know
          Fernanda is working on a patch for that. Eventually is will be a
          "pop", right now maximum one warning is being transmitted.

          Øystein> - Why do you have to delay the call to setIsUpdataDeleteHole until
          Øystein> later and not do it when testing for warnings?

          The existing code checked for update/delete holes by relying only
          nulldata, which is parsed later than the warning. For SUR, in addition
          to nulldata, we send a warning (SQLState.ROW_DELETED) since this is
          required by DRDA, cf the write-up section on this. The existing code
          is probably non-compliant. Rather than calling setIsUpdataDeleteHole()
          in two places, we chose to use the existing code location for calling
          it, using the helper variable receivedDeleteHoleWarning in the SUR
          case. I agree the code could be simplified by removing the old logic
          (relying only on nulldata), but we chose the conservative approach.

          Øystein>
          Øystein> 5. NetStatementReply
          Øystein>
          Øystein> - Call to ClientJDBCObjectFactory.newNetResultSet() lists possible
          Øystein> values. This needs to be updated to include QRYSNSSTC.

          Agreed, done.

          Øystein>
          Øystein> 6. Statement
          Øystein>
          Øystein> - Why have you made warnings_ protected? It seems like it is package
          Øystein> private that is needed. Why not keep it private and supply a
          Øystein> get-method?

          Agreed, done. The old code is too not good on encapsulation, so it
          leads one astray Introduced a protected method
          Statement#getWarnings_, an accessor for warnings_, which is now
          private.

          Øystein>
          Øystein> 7. ResultSet
          Øystein>
          Øystein> - Why have you made suggestedFetchSize_ public? It seems like it
          Øystein> could be protected.

          Yes. I made it protected.

          Øystein>
          Øystein> - Comments for fetchSize_ and suggestedFetchSize_ are a bit brief.

          Expanded those comments, included here for your convenience:

          // Gets its initial value from the statement when the result set is created.
          // It can be modified by setFetchSize and retrieved via getFetchSize.
          protected int suggestedFetchSize_;

          // Set by the net layer based on suggestedFetchSize_, protocol
          // type, scrollability and presence of lobs.
          public int fetchSize_;

          Øystein> (And if they are to be public, you should supply javadoc).
          Øystein>
          Øystein> - relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for
          Øystein> all result sets?

          For SUR, the check is performed by the getAbsoluteRowset.

          Øystein> - relativex(): It seems like the code to call getAbsoluteResultSet is
          Øystein> duplicated. I do not think the second call will ever be executed.

          Yes, you are right. Vestige from a re-write. I removed this.

          Øystein>
          Øystein> - rowUpdated(): Why not always call getIsRowUpdated? I would
          Øystein> think it was valid for all result sets.

          This method used to return false for all result sets, since
          detectability was not implemented in Derby (for any result set). This
          patch implements it for SUR, so getIsRowUpdated is only called for
          SUR. I changed this as you suggest, making sure it will return false
          in non-SUR cases.

          Øystein>
          Øystein> - rowUpdated()/rowDeleted(): Thss methods are a bit asymmetric.
          Øystein> rowUpdated() calls a get-method while rowDeleted accesses
          Øystein> isUpdateDeleteHole_ directly.

          The old code had the isUpdateDeleteHole as "public" and I tried to
          make patch as small as possible, but I agree it's asymmetric. I
          changed the access into using a getter method.

          Øystein> - updateRow(): I tried to browse the spec and the javadoc and I did
          Øystein> not find anything that said that there is a difference
          Øystein> between forward only cursors and scrollable cursors with respect to
          Øystein> validity of the current position after and update. Where is this
          Øystein> stated?

          This is not mandated by JDBC. I remember asking Dan about it once, but
          I don't rememeber the rationale for making forward only cursors lose
          the position after an updateRow. I checked the SQL 2003 standard just
          now, but I could not find it is a requirement for positioned update,
          either.

          Øystein>
          Øystein> - checkForUpdatableResultSet(): What is the difference between this
          Øystein> new method and the existing checkUpdatableCursor()? It seems better
          Øystein> to change the existing than to make a new method.

          Merged these methods into one and updated masters to match the changed
          (and better) exceptions.

          Show
          Dag H. Wanvik added a comment - Here are answers to Øysteins comments and questions. A new version of the patch (#3) reflecting those answers has been uploaded. In this version, some too long lines added or changed have also been normalized (<= 80 chars wide). I also fixed added or changed lines to comply with surrounding whitespace styles (tab or not). I ran derbyall using Solaris 10/x86 Sun JRE 1.4.2 with no untoward results. >>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote: Øystein> 1. metadata_net.properties: Øystein> Øystein> - Is the format and constant values used here documented anywhere? As part of DERBY-965 , I added some comments explaining the syntax of the fields I touched, please see the file from line 197 onwards. I am not aware of any other documentation. Øystein> 2. DRDAConnThread Øystein> Øystein> - parseSQLATTR(): Variable insensitive can be removed entirely. It Øystein> serves no purpose anymore. Agreed, removed. Øystein> Øystein> 3. NetResultSet/NetResultSet40 Øystein> Øystein> - Parameters of constructor has comments with possible values. This Øystein> needs to be updated to include QRYSNSSTC. Agreed, done. Øystein> 4. NetCursor Øystein> Øystein> - The writeup says the purpose is to pop warnings, but the code seems Øystein> to only handle a single warning. Currently, chained warnings are not being sent over DRDA. I know Fernanda is working on a patch for that. Eventually is will be a "pop", right now maximum one warning is being transmitted. Øystein> - Why do you have to delay the call to setIsUpdataDeleteHole until Øystein> later and not do it when testing for warnings? The existing code checked for update/delete holes by relying only nulldata, which is parsed later than the warning. For SUR, in addition to nulldata, we send a warning (SQLState.ROW_DELETED) since this is required by DRDA, cf the write-up section on this. The existing code is probably non-compliant. Rather than calling setIsUpdataDeleteHole() in two places, we chose to use the existing code location for calling it, using the helper variable receivedDeleteHoleWarning in the SUR case. I agree the code could be simplified by removing the old logic (relying only on nulldata), but we chose the conservative approach. Øystein> Øystein> 5. NetStatementReply Øystein> Øystein> - Call to ClientJDBCObjectFactory.newNetResultSet() lists possible Øystein> values. This needs to be updated to include QRYSNSSTC. Agreed, done. Øystein> Øystein> 6. Statement Øystein> Øystein> - Why have you made warnings_ protected? It seems like it is package Øystein> private that is needed. Why not keep it private and supply a Øystein> get-method? Agreed, done. The old code is too not good on encapsulation, so it leads one astray Introduced a protected method Statement#getWarnings_, an accessor for warnings_, which is now private. Øystein> Øystein> 7. ResultSet Øystein> Øystein> - Why have you made suggestedFetchSize_ public? It seems like it Øystein> could be protected. Yes. I made it protected. Øystein> Øystein> - Comments for fetchSize_ and suggestedFetchSize_ are a bit brief. Expanded those comments, included here for your convenience: // Gets its initial value from the statement when the result set is created. // It can be modified by setFetchSize and retrieved via getFetchSize. protected int suggestedFetchSize_; // Set by the net layer based on suggestedFetchSize_, protocol // type, scrollability and presence of lobs. public int fetchSize_; Øystein> (And if they are to be public, you should supply javadoc). Øystein> Øystein> - relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for Øystein> all result sets? For SUR, the check is performed by the getAbsoluteRowset. Øystein> - relativex(): It seems like the code to call getAbsoluteResultSet is Øystein> duplicated. I do not think the second call will ever be executed. Yes, you are right. Vestige from a re-write. I removed this. Øystein> Øystein> - rowUpdated(): Why not always call getIsRowUpdated? I would Øystein> think it was valid for all result sets. This method used to return false for all result sets, since detectability was not implemented in Derby (for any result set). This patch implements it for SUR, so getIsRowUpdated is only called for SUR. I changed this as you suggest, making sure it will return false in non-SUR cases. Øystein> Øystein> - rowUpdated()/rowDeleted(): Thss methods are a bit asymmetric. Øystein> rowUpdated() calls a get-method while rowDeleted accesses Øystein> isUpdateDeleteHole_ directly. The old code had the isUpdateDeleteHole as "public" and I tried to make patch as small as possible, but I agree it's asymmetric. I changed the access into using a getter method. Øystein> - updateRow(): I tried to browse the spec and the javadoc and I did Øystein> not find anything that said that there is a difference Øystein> between forward only cursors and scrollable cursors with respect to Øystein> validity of the current position after and update. Where is this Øystein> stated? This is not mandated by JDBC. I remember asking Dan about it once, but I don't rememeber the rationale for making forward only cursors lose the position after an updateRow. I checked the SQL 2003 standard just now, but I could not find it is a requirement for positioned update, either. Øystein> Øystein> - checkForUpdatableResultSet(): What is the difference between this Øystein> new method and the existing checkUpdatableCursor()? It seems better Øystein> to change the existing than to make a new method. Merged these methods into one and updated masters to match the changed (and better) exceptions.
          Hide
          Øystein Grøvlen added a comment -

          Changes looks good. Just a few follow-up comments/questions:

          Dag H. Wanvik (JIRA) wrote:
          >>>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote:
          > Øystein> 1. metadata_net.properties:
          > Øystein>
          > Øystein> - Is the format and constant values used here documented anywhere?
          >
          > As part of DERBY-965, I added some comments explaining the syntax of
          > the fields I touched, please see the file from line 197 onwards. I
          > am not aware of any other documentation.

          What about the constants like 1004. How did you figure out which
          values to use?

          > Øystein> 4. NetCursor
          >
          > Øystein> - Why do you have to delay the call to setIsUpdataDeleteHole until
          > Øystein> later and not do it when testing for warnings?
          >
          > The existing code checked for update/delete holes by relying only
          > nulldata, which is parsed later than the warning. For SUR, in addition
          > to nulldata, we send a warning (SQLState.ROW_DELETED) since this is
          > required by DRDA, cf the write-up section on this. The existing code
          > is probably non-compliant. Rather than calling setIsUpdataDeleteHole()
          > in two places, we chose to use the existing code location for calling
          > it, using the helper variable receivedDeleteHoleWarning in the SUR
          > case. I agree the code could be simplified by removing the old logic
          > (relying only on nulldata), but we chose the conservative approach.

          It might be conservative, but it is not very cautious. It seems like
          you silently ignore scenarios where you get both a warning and data,
          and the case where you get no data and no warning. Should not these
          scenarios generate an error or at least be handled by an assert?

          >
          > Øystein>
          > Øystein> 6. Statement
          > Øystein>
          > Øystein> - Why have you made warnings_ protected? It seems like it is package
          > Øystein> private that is needed. Why not keep it private and supply a
          > Øystein> get-method?
          >
          > Agreed, done. The old code is too not good on encapsulation, so it
          > leads one astray Introduced a protected method
          > Statement#getWarnings_, an accessor for warnings_, which is now
          > private.

          '_'-suffix for method names. Is that common?

          > Øystein> - relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for
          > Øystein> all result sets?
          >
          > For SUR, the check is performed by the getAbsoluteRowset.

          Is there a particular reason why this code cannot be common to all
          result sets?

          > Øystein> - updateRow(): I tried to browse the spec and the javadoc and I did
          > Øystein> not find anything that said that there is a difference
          > Øystein> between forward only cursors and scrollable cursors with respect to
          > Øystein> validity of the current position after and update. Where is this
          > Øystein> stated?
          >
          > This is not mandated by JDBC. I remember asking Dan about it once, but
          > I don't rememeber the rationale for making forward only cursors lose
          > the position after an updateRow. I checked the SQL 2003 standard just
          > now, but I could not find it is a requirement for positioned update,
          > either.

          To me it seems like a common behavior for forward-only and scrollable
          cursors should be more important than a specific behavior for any of
          them.

          >
          > Øystein>
          > Øystein> - checkForUpdatableResultSet(): What is the difference between this
          > Øystein> new method and the existing checkUpdatableCursor()? It seems better
          > Øystein> to change the existing than to make a new method.
          >
          > Merged these methods into one and updated masters to match the changed
          > (and better) exceptions.

          Good. I see there are few other places that could have used this
          method (insertRow(), refreshRow(), checkUpdatePreconditions(). Either
          you could change that, or we should send a note to David to make him
          aware of it for his internationalization work.

          Show
          Øystein Grøvlen added a comment - Changes looks good. Just a few follow-up comments/questions: Dag H. Wanvik (JIRA) wrote: >>>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote: > Øystein> 1. metadata_net.properties: > Øystein> > Øystein> - Is the format and constant values used here documented anywhere? > > As part of DERBY-965 , I added some comments explaining the syntax of > the fields I touched, please see the file from line 197 onwards. I > am not aware of any other documentation. What about the constants like 1004. How did you figure out which values to use? > Øystein> 4. NetCursor > > Øystein> - Why do you have to delay the call to setIsUpdataDeleteHole until > Øystein> later and not do it when testing for warnings? > > The existing code checked for update/delete holes by relying only > nulldata, which is parsed later than the warning. For SUR, in addition > to nulldata, we send a warning (SQLState.ROW_DELETED) since this is > required by DRDA, cf the write-up section on this. The existing code > is probably non-compliant. Rather than calling setIsUpdataDeleteHole() > in two places, we chose to use the existing code location for calling > it, using the helper variable receivedDeleteHoleWarning in the SUR > case. I agree the code could be simplified by removing the old logic > (relying only on nulldata), but we chose the conservative approach. It might be conservative, but it is not very cautious. It seems like you silently ignore scenarios where you get both a warning and data, and the case where you get no data and no warning. Should not these scenarios generate an error or at least be handled by an assert? > > Øystein> > Øystein> 6. Statement > Øystein> > Øystein> - Why have you made warnings_ protected? It seems like it is package > Øystein> private that is needed. Why not keep it private and supply a > Øystein> get-method? > > Agreed, done. The old code is too not good on encapsulation, so it > leads one astray Introduced a protected method > Statement#getWarnings_, an accessor for warnings_, which is now > private. '_'-suffix for method names. Is that common? > Øystein> - relativex(): Why do not the isBeforeFirstX()/isAfterLastX() test for > Øystein> all result sets? > > For SUR, the check is performed by the getAbsoluteRowset. Is there a particular reason why this code cannot be common to all result sets? > Øystein> - updateRow(): I tried to browse the spec and the javadoc and I did > Øystein> not find anything that said that there is a difference > Øystein> between forward only cursors and scrollable cursors with respect to > Øystein> validity of the current position after and update. Where is this > Øystein> stated? > > This is not mandated by JDBC. I remember asking Dan about it once, but > I don't rememeber the rationale for making forward only cursors lose > the position after an updateRow. I checked the SQL 2003 standard just > now, but I could not find it is a requirement for positioned update, > either. To me it seems like a common behavior for forward-only and scrollable cursors should be more important than a specific behavior for any of them. > > Øystein> > Øystein> - checkForUpdatableResultSet(): What is the difference between this > Øystein> new method and the existing checkUpdatableCursor()? It seems better > Øystein> to change the existing than to make a new method. > > Merged these methods into one and updated masters to match the changed > (and better) exceptions. Good. I see there are few other places that could have used this method (insertRow(), refreshRow(), checkUpdatePreconditions(). Either you could change that, or we should send a note to David to make him aware of it for his internationalization work.
          Hide
          Øystein Grøvlen added a comment -

          I have had a look at the test changes in this patch. I realize most
          of my comments are general comments to SUR-tests rather than
          particular to this patch. Hence, I do not think it needs to be
          resolved before this patch is checked in.

          8. SURBaseTest

          • assertWarning(): Why is the SQLState not tested for
            DerbyNetClientFramework()? Would it not be more general to skip the
            test if SQLState was null?

          9. SURQueryMixTest

          • I do not think the Javadoc for testNavigation is up-to-date. It
            says that it requires that the resultset is position before the
            first row, but starts with a call to afterLast(). It is a void
            function, but Javadoc says it returns a map.
          • It seems like this test is based on making an update, read the
            updated row into a map and use that for comparison when you
            renavigate to that row. This assumes that what you read after an
            update reflects the updates just made. Do you have a test that
            tests that the intended update is actually reflected?

          10. HoldabilityTest

          • Why do you not check warnings for isDerbyNetClientFramework?

          11. SURTest

          • Why are some of the tests on forward-only result sets and not on
            scrollable?

          12. updatableResultSet

          • I see from the master files that different SQLStates are used in the
            embedded versus in the client. E.g., 01J02 vs. 01X02, 24000 vs.
            XCL07/XCL08/XCL16. Is that the way it is supposed to be?
          Show
          Øystein Grøvlen added a comment - I have had a look at the test changes in this patch. I realize most of my comments are general comments to SUR-tests rather than particular to this patch. Hence, I do not think it needs to be resolved before this patch is checked in. 8. SURBaseTest assertWarning(): Why is the SQLState not tested for DerbyNetClientFramework()? Would it not be more general to skip the test if SQLState was null? 9. SURQueryMixTest I do not think the Javadoc for testNavigation is up-to-date. It says that it requires that the resultset is position before the first row, but starts with a call to afterLast(). It is a void function, but Javadoc says it returns a map. It seems like this test is based on making an update, read the updated row into a map and use that for comparison when you renavigate to that row. This assumes that what you read after an update reflects the updates just made. Do you have a test that tests that the intended update is actually reflected? 10. HoldabilityTest Why do you not check warnings for isDerbyNetClientFramework? 11. SURTest Why are some of the tests on forward-only result sets and not on scrollable? 12. updatableResultSet I see from the master files that different SQLStates are used in the embedded versus in the client. E.g., 01J02 vs. 01X02, 24000 vs. XCL07/XCL08/XCL16. Is that the way it is supposed to be?
          Hide
          Dag H. Wanvik added a comment -

          Answers to Øystein's latest comments and questions. I synced up to svn
          392313 and ran derbyall again (Solaris 10/x86, Sun JDK 1.4.2).

          >>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote:

          Øystein> What about the constants like 1004. How did you figure out
          Øystein> which values to use?

          These constants are from the Java ResultSet interface:

          ../jdk1.5.0/src.zip:java/sql/ResultSet.java:
          :
          int TYPE_FORWARD_ONLY = 1003;
          :

          Øystein> It might be conservative, but it is not very cautious. It
          Øystein> seems like you silently ignore scenarios where you get both a
          Øystein> warning and data, and the case where you get no data and no
          Øystein> warning. Should not these scenarios generate an error or at
          Øystein> least be handled by an assert?

          I think changing existing code semantics is less cautious than keeping
          it But I agree it could be useful to include an assert of the new
          case ensuring that there is nulldata whenever we get the new warning
          (SUR scenario). Did that. For the "old" case, I am not certain it
          could never happen in some scenario, so I won't add an assert for
          that.

          Øystein>
          Øystein> '_'-suffix for method names. Is that common?

          There is already a API method called getWarnings, so I added a suffix
          to indicate that this is an internal method. But you are right, this
          suffix is not in frequent use, so I have changed the name to
          getSqlWarnings.

          Øystein> Is there a particular reason why this code cannot be common
          Øystein> to all result sets?

          I guess not, but I think the call to getAbsoluteRowset is necessary
          and sufficient.

          Øystein> To me it seems like a common behavior for forward-only and
          Øystein> scrollable cursors should be more important than a specific
          Øystein> behavior for any of them.

          I guess in a forward-only updatable result set, the expectation would
          be that after updating a row you would want to reposition to the next
          row always.

          In a scrollable result set we have no a priori direction in which to
          move after an update, so IMHO it does not seem logical to inherit the
          behavior of forward-only for scrollable.

          If anything, I would suggest removing this behavior for
          forward-only. Anyway, the embedded SUR does not require repositioning
          either, so if you want this changed, you should log a JIRA issue, I
          think.

          Øystein> Good. I see there are few other places that could have used
          Øystein> this method (insertRow(), refreshRow(),
          Øystein> checkUpdatePreconditions(). Either you could change that, or
          Øystein> we should send a note to David to make him aware of it for
          Øystein> his internationalization work.

          I agree. Although it might make as much sense do do this as part of a
          general clean-up, I changed those to use the same method
          (checkForUpdatableResultSet), as well, since it moves in the right
          direction and is SUR-related.

          Show
          Dag H. Wanvik added a comment - Answers to Øystein's latest comments and questions. I synced up to svn 392313 and ran derbyall again (Solaris 10/x86, Sun JDK 1.4.2). >>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote: Øystein> What about the constants like 1004. How did you figure out Øystein> which values to use? These constants are from the Java ResultSet interface: ../jdk1.5.0/src.zip:java/sql/ResultSet.java: : int TYPE_FORWARD_ONLY = 1003; : Øystein> It might be conservative, but it is not very cautious. It Øystein> seems like you silently ignore scenarios where you get both a Øystein> warning and data, and the case where you get no data and no Øystein> warning. Should not these scenarios generate an error or at Øystein> least be handled by an assert? I think changing existing code semantics is less cautious than keeping it But I agree it could be useful to include an assert of the new case ensuring that there is nulldata whenever we get the new warning (SUR scenario). Did that. For the "old" case, I am not certain it could never happen in some scenario, so I won't add an assert for that. Øystein> Øystein> '_'-suffix for method names. Is that common? There is already a API method called getWarnings, so I added a suffix to indicate that this is an internal method. But you are right, this suffix is not in frequent use, so I have changed the name to getSqlWarnings. Øystein> Is there a particular reason why this code cannot be common Øystein> to all result sets? I guess not, but I think the call to getAbsoluteRowset is necessary and sufficient. Øystein> To me it seems like a common behavior for forward-only and Øystein> scrollable cursors should be more important than a specific Øystein> behavior for any of them. I guess in a forward-only updatable result set, the expectation would be that after updating a row you would want to reposition to the next row always. In a scrollable result set we have no a priori direction in which to move after an update, so IMHO it does not seem logical to inherit the behavior of forward-only for scrollable. If anything, I would suggest removing this behavior for forward-only. Anyway, the embedded SUR does not require repositioning either, so if you want this changed, you should log a JIRA issue, I think. Øystein> Good. I see there are few other places that could have used Øystein> this method (insertRow(), refreshRow(), Øystein> checkUpdatePreconditions(). Either you could change that, or Øystein> we should send a note to David to make him aware of it for Øystein> his internationalization work. I agree. Although it might make as much sense do do this as part of a general clean-up, I changed those to use the same method (checkForUpdatableResultSet), as well, since it moves in the right direction and is SUR-related.
          Hide
          Andreas Korneliussen added a comment -

          This patch (derby-775-5.diff) addresses comments about the test changes. The patch is a modification of derby-775-4.diff.

          The test changes are:

          SURBaseTest:

          • assertWarning: asserts that warning has correct SQLState. If the warning is null, this is accepted if not in embedded framework.

          SURQueryMixTest:

          • Previously the test would accept that the ResultSet got concurrency mode CONCUR_READ_ONLY and skip update testing. Now it will assert here, to prevent regression.
          • updated javadoc for testNavigation

          HoldabilityTest:

          • uses assertWarning from SURBaseTest. It also asserts on concurrency mode, instead of skipping the update part, to prevent regression.
          Show
          Andreas Korneliussen added a comment - This patch (derby-775-5.diff) addresses comments about the test changes. The patch is a modification of derby-775-4.diff. The test changes are: SURBaseTest: assertWarning: asserts that warning has correct SQLState. If the warning is null, this is accepted if not in embedded framework. SURQueryMixTest: Previously the test would accept that the ResultSet got concurrency mode CONCUR_READ_ONLY and skip update testing. Now it will assert here, to prevent regression. updated javadoc for testNavigation HoldabilityTest: uses assertWarning from SURBaseTest. It also asserts on concurrency mode, instead of skipping the update part, to prevent regression.
          Hide
          Øystein Grøvlen added a comment -

          Changes looks good. I have follow-up comments on a two issues:

          Dag H. Wanvik (JIRA) wrote:
          > Øystein> least be handled by an assert?
          >
          > I think changing existing code semantics is less cautious than keeping
          > it But I agree it could be useful to include an assert of the new
          > case ensuring that there is nulldata whenever we get the new warning
          > (SUR scenario). Did that. For the "old" case, I am not certain it
          > could never happen in some scenario, so I won't add an assert for
          > that.
          >

          That you are not certain that it could never happen, worries me even
          more since you have disabled the handling of this case. Earlier, this
          case would result in a "delete hole". Now, it is just ignored. What
          will happen if a client tries to access the current record?

          >
          > Øystein> To me it seems like a common behavior for forward-only and
          > Øystein> scrollable cursors should be more important than a specific
          > Øystein> behavior for any of them.
          >
          > I guess in a forward-only updatable result set, the expectation would
          > be that after updating a row you would want to reposition to the next
          > row always.
          >
          > In a scrollable result set we have no a priori direction in which to
          > move after an update, so IMHO it does not seem logical to inherit the
          > behavior of forward-only for scrollable.

          I cannot see how direction is relevant. This is about being able to
          access the current record after it has been updated before any
          navigation. I do not understand why that should make more or less
          sense for SUR than for forward-only result sets.

          >
          > If anything, I would suggest removing this behavior for
          > forward-only. Anyway, the embedded SUR does not require repositioning
          > either, so if you want this changed, you should log a JIRA issue, I
          > think.

          I think you are saying that for SUR it makes most sense to still be
          able to access the current record after it has been updated. I agree,
          and I also agree that would be the ideal solution for forward-only.
          However, given that forward-only has a different behavior, my question
          is why you think it is more important to implement the proposed
          behavior for SUR than to have a common behavior for both?

          Show
          Øystein Grøvlen added a comment - Changes looks good. I have follow-up comments on a two issues: Dag H. Wanvik (JIRA) wrote: > Øystein> least be handled by an assert? > > I think changing existing code semantics is less cautious than keeping > it But I agree it could be useful to include an assert of the new > case ensuring that there is nulldata whenever we get the new warning > (SUR scenario). Did that. For the "old" case, I am not certain it > could never happen in some scenario, so I won't add an assert for > that. > That you are not certain that it could never happen, worries me even more since you have disabled the handling of this case. Earlier, this case would result in a "delete hole". Now, it is just ignored. What will happen if a client tries to access the current record? > > Øystein> To me it seems like a common behavior for forward-only and > Øystein> scrollable cursors should be more important than a specific > Øystein> behavior for any of them. > > I guess in a forward-only updatable result set, the expectation would > be that after updating a row you would want to reposition to the next > row always. > > In a scrollable result set we have no a priori direction in which to > move after an update, so IMHO it does not seem logical to inherit the > behavior of forward-only for scrollable. I cannot see how direction is relevant. This is about being able to access the current record after it has been updated before any navigation. I do not understand why that should make more or less sense for SUR than for forward-only result sets. > > If anything, I would suggest removing this behavior for > forward-only. Anyway, the embedded SUR does not require repositioning > either, so if you want this changed, you should log a JIRA issue, I > think. I think you are saying that for SUR it makes most sense to still be able to access the current record after it has been updated. I agree, and I also agree that would be the ideal solution for forward-only. However, given that forward-only has a different behavior, my question is why you think it is more important to implement the proposed behavior for SUR than to have a common behavior for both?
          Hide
          Dag H. Wanvik added a comment -

          A new version of the patch. Changes details

          • Merge conflict with David's changes for am/ResultSet.java have
            been resolved, in some cases by keeping his changes and dropping
            similar changes in the previous version of this patch, in other
            cases vice versa. Notably this patch in a few places replaces
            XJ086 with XJ083. This makes for more precise indication on
            offending operation and more similarity to embedded). David, it
            would be nice if you could have a look at this to see if it is ok
            with you.
          • Fixed a wrong operation string (cancelRowUpdates) to
            checkForUpdatableResultSet.
          • An added sane assertion for the case Øystein asked for in
            net/NetCursor.java
          • A fix to make sure detectability for rowDeleted isn't introduced
            for FORWARD_ONLY result sets (side-effect of offering it for SUR).
          • Removed a test comment line no longer pertinent in
            lang/updatableResultSet.java

          I have run derbyall for Solaris 10/x86 with Sun JDK 1.5 again.

          Answers and comments to Øystein's latest comments.

          >>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote:

          Øystein> That you are not certain that it could never happen, worries
          Øystein> me even more since you have disabled the handling of this
          Øystein> case. Earlier, this case would result in a "delete hole".
          Øystein> Now, it is just ignored. What will happen if a client tries
          Øystein> to access the current record?

          You are right, I added a sane mode invariant check.

          Øystein> I cannot see how direction is relevant. This is about being
          Øystein> able to access the current record after it has been updated
          Øystein> before any navigation. I do not understand why that should
          Øystein> make more or less sense for SUR than for forward-only result
          Øystein> sets.

          I was just trying to guess at why the restriction was introduced or
          why it was felt acceptable. My thought was it might be easier to
          specify the cursor's whereabouts in the forward only case ("just
          before next row").

          Øystein> I think you are saying that for SUR it makes most sense to
          Øystein> still be able to access the current record after it has been
          Øystein> updated. I agree, and I also agree that would be the ideal
          Øystein> solution for forward-only. However, given that forward-only
          Øystein> has a different behavior, my question is why you think it is
          Øystein> more important to implement the proposed behavior for SUR
          Øystein> than to have a common behavior for both?

          I guess you could argue that keeping the forward-only behavior and
          then possibly changing both FO and SUR at a later date is a better
          (more conservative) approach. But lifting the requirement should
          hopefully not prove too problematic for the end user. Anyway, this
          patch is about making the client version of SUR; embedded behaves the
          same way, so I suggest we don't change this for now. If you think SUR
          should be changed to follow the semantics of forward-only, please file
          a JIRA for it. Alternately we should file a JIRA to the lift the
          requirement for FO unless there is a good reason to keep it the way it
          is.

          Show
          Dag H. Wanvik added a comment - A new version of the patch. Changes details Merge conflict with David's changes for am/ResultSet.java have been resolved, in some cases by keeping his changes and dropping similar changes in the previous version of this patch, in other cases vice versa. Notably this patch in a few places replaces XJ086 with XJ083. This makes for more precise indication on offending operation and more similarity to embedded). David, it would be nice if you could have a look at this to see if it is ok with you. Fixed a wrong operation string (cancelRowUpdates) to checkForUpdatableResultSet. An added sane assertion for the case Øystein asked for in net/NetCursor.java A fix to make sure detectability for rowDeleted isn't introduced for FORWARD_ONLY result sets (side-effect of offering it for SUR). Removed a test comment line no longer pertinent in lang/updatableResultSet.java I have run derbyall for Solaris 10/x86 with Sun JDK 1.5 again. Answers and comments to Øystein's latest comments. >>>>> "Øystein" == Øystein Grøvlen (JIRA) <derby-dev@db.apache.org> wrote: Øystein> That you are not certain that it could never happen, worries Øystein> me even more since you have disabled the handling of this Øystein> case. Earlier, this case would result in a "delete hole". Øystein> Now, it is just ignored. What will happen if a client tries Øystein> to access the current record? You are right, I added a sane mode invariant check. Øystein> I cannot see how direction is relevant. This is about being Øystein> able to access the current record after it has been updated Øystein> before any navigation. I do not understand why that should Øystein> make more or less sense for SUR than for forward-only result Øystein> sets. I was just trying to guess at why the restriction was introduced or why it was felt acceptable. My thought was it might be easier to specify the cursor's whereabouts in the forward only case ("just before next row"). Øystein> I think you are saying that for SUR it makes most sense to Øystein> still be able to access the current record after it has been Øystein> updated. I agree, and I also agree that would be the ideal Øystein> solution for forward-only. However, given that forward-only Øystein> has a different behavior, my question is why you think it is Øystein> more important to implement the proposed behavior for SUR Øystein> than to have a common behavior for both? I guess you could argue that keeping the forward-only behavior and then possibly changing both FO and SUR at a later date is a better (more conservative) approach. But lifting the requirement should hopefully not prove too problematic for the end user. Anyway, this patch is about making the client version of SUR; embedded behaves the same way, so I suggest we don't change this for now. If you think SUR should be changed to follow the semantics of forward-only, please file a JIRA for it. Alternately we should file a JIRA to the lift the requirement for FO unless there is a good reason to keep it the way it is.
          Hide
          David Van Couvering added a comment -

          Hi, Dag. These changes look fine to me. I generally believe you know better than I what the appropriate message is. I'm going through completely out of context trying to convert these hardcoded strings to internationalized strings, but you actually know what's going on in the code

          Show
          David Van Couvering added a comment - Hi, Dag. These changes look fine to me. I generally believe you know better than I what the appropriate message is. I'm going through completely out of context trying to convert these hardcoded strings to internationalized strings, but you actually know what's going on in the code
          Hide
          Øystein Grøvlen added a comment -

          Changes look good. I think this patch is ready for commit.

          I agree that at this stage it makes most sense to leave the issue of validity of current row after an update to a separate Jira issue.

          Show
          Øystein Grøvlen added a comment - Changes look good. I think this patch is ready for commit. I agree that at this stage it makes most sense to leave the issue of validity of current row after an update to a separate Jira issue.
          Hide
          Dag H. Wanvik added a comment -

          Patch did not apply correctly any longer, fixed a merge conflict with spilitmessages.java.
          Made sure it applies and compiles again.

          Show
          Dag H. Wanvik added a comment - Patch did not apply correctly any longer, fixed a merge conflict with spilitmessages.java. Made sure it applies and compiles again.
          Hide
          Bernt M. Johnsen added a comment -

          Committed revision 395866.

          Show
          Bernt M. Johnsen added a comment - Committed revision 395866.

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Dag H. Wanvik
            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development