Uploaded image for project: 'Derby'
  1. Derby
  2. DERBY-3573

Argument checking for ResultSet.setFetchSize(int) is incorrect

    Details

    • Urgency:
      Low
    • Issue & fix info:
      Newcomer

      Description

      The requirement that the argument to ResultSet.setFetchSize(int) be less than Statement.getMaxRows() was dropped in Java 6/JDBC 4, (it is not present in the Java 6 javadoc, but can still be seen in the Java 5 javadoc).

      The reason why the client driver doesn't throw an exception in this case is because am.ResultSet incorrectly checks against ResultSet.maxRows_ and NOT am.Statement.getMaxRows(). So when am.Statement.setMaxRows(int) is called after a result set has already been created, am.ResultSet.setFechSize(int) will check against a stale value.

      The question is what to do about this. The client driver clearly has a bug, but should we fix it by duplicating the old behavior found in the embedded driver, or change both drivers to comply with latest spec which allows any non-negative value as argument to ResultSet.setFetchSize(int)?

      1. derby-3573-test.diff
        1 kB
        Dyre Tjeldvoll
      2. derby-3573-b.diff
        2 kB
        Dyre Tjeldvoll
      3. derby-3573-a.diff
        2 kB
        Nirmal Fernando

        Issue Links

          Activity

          Hide
          espinha Tiago R. Espinha added a comment -

          Triaged for 10.5.2. No changes made.

          Show
          espinha Tiago R. Espinha added a comment - Triaged for 10.5.2. No changes made.
          Hide
          kmarsden Kathey Marsden added a comment -

          Triage for 10.8. I think it makes sense to change both drivers to comply with latest spec which allows any non-negative value as argument to ResultSet.setFetchSize(int). Does this mean that it will override setMaxRows()?

          Show
          kmarsden Kathey Marsden added a comment - Triage for 10.8. I think it makes sense to change both drivers to comply with latest spec which allows any non-negative value as argument to ResultSet.setFetchSize(int). Does this mean that it will override setMaxRows()?
          Hide
          dagw Dag H. Wanvik added a comment - - edited

          +1 to making both drivers comply. I think setFetchSize and setMaxRows are orthogonal, but if setFetchSize is set greater than setMaxRows, I think the setting in setMaxRows still apply: the "fetch size" (a "hint") is about how many rows to fetch at a time from the server to the client (trade-off between network round-tips, client memory size, latency for first rows) as far as I understand it. "Max size" is a hard limit for how many rows are visible as part of the result set, no matter whether this is enforced on server or client side.

          Show
          dagw Dag H. Wanvik added a comment - - edited +1 to making both drivers comply. I think setFetchSize and setMaxRows are orthogonal, but if setFetchSize is set greater than setMaxRows, I think the setting in setMaxRows still apply: the "fetch size" (a "hint") is about how many rows to fetch at a time from the server to the client (trade-off between network round-tips, client memory size, latency for first rows) as far as I understand it. "Max size" is a hard limit for how many rows are visible as part of the result set, no matter whether this is enforced on server or client side.
          Hide
          nirmal Nirmal Fernando added a comment -

          Attaching a diff!
          Is there a way to find test files related to this change?

          Thanks.

          Show
          nirmal Nirmal Fernando added a comment - Attaching a diff! Is there a way to find test files related to this change? Thanks.
          Hide
          knutanders Knut Anders Hatlen added a comment -

          StatementJdbc20Test.java has some tests for rs.setFetchSize().

          Show
          knutanders Knut Anders Hatlen added a comment - StatementJdbc20Test.java has some tests for rs.setFetchSize().
          Hide
          dyret Dyre Tjeldvoll added a comment -

          Patch a no longer applied cleanly, so attaching an identical patch against the latest trunk. Junit and harness tests passed.

          Show
          dyret Dyre Tjeldvoll added a comment - Patch a no longer applied cleanly, so attaching an identical patch against the latest trunk. Junit and harness tests passed.
          Hide
          dyret Dyre Tjeldvoll added a comment -

          StatementJdbc20Test passed, but only because there was no fail() after setting a fetchSize larger than maxRows. Attaching a test patch to get the new behavior tested.

          Show
          dyret Dyre Tjeldvoll added a comment - StatementJdbc20Test passed, but only because there was no fail() after setting a fetchSize larger than maxRows. Attaching a test patch to get the new behavior tested.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1531854 from Dyre Tjeldvoll in branch 'code/trunk'
          [ https://svn.apache.org/r1531854 ]

          DERBY-3573: Remove incorrect check against maxRows when setting fetchSize. Patch contributed by Nirmal Fernando.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1531854 from Dyre Tjeldvoll in branch 'code/trunk' [ https://svn.apache.org/r1531854 ] DERBY-3573 : Remove incorrect check against maxRows when setting fetchSize. Patch contributed by Nirmal Fernando.
          Hide
          mikem Mike Matrigali added a comment -

          consider for 10.10 backport

          Show
          mikem Mike Matrigali added a comment - consider for 10.10 backport
          Hide
          mikem Mike Matrigali added a comment -

          Marking not for backport. Change seems like it might affect existing applications, so better to introduce the change to implement the new standard only in a new release.

          Show
          mikem Mike Matrigali added a comment - Marking not for backport. Change seems like it might affect existing applications, so better to introduce the change to implement the new standard only in a new release.

            People

            • Assignee:
              nirmal Nirmal Fernando
              Reporter:
              dyret Dyre Tjeldvoll
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development