Derby
  1. Derby
  2. DERBY-4198

When using the FOR UPDATE OF clause with SUR (Scroll-insensive updatable result sets), the updateRow() method crashes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.2.1.6, 10.2.2.0, 10.3.1.4, 10.3.2.1, 10.3.3.0, 10.4.1.3, 10.4.2.0, 10.5.1.1
    • Fix Version/s: 10.5.2.0, 10.6.1.0
    • Component/s: JDBC, Network Client, SQL
    • Labels:
      None
    • Urgency:
      Normal
    • Bug behavior facts:
      Crash

      Description

      This problem occurs on both Client/Server and Embedded.

      With the Embedded driver, the JVM crashes with the following error:
      ------------------------------------8<-----------------------------------
      1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)org.apache.derby.shared.common.sanity.AssertFailure: ASSERT FAILED statementContext is not expected to equal statementContexts[0]
      at org.apache.derby.shared.common.sanity.SanityManager.ASSERT(SanityManager.java:120)
      at org.apache.derby.impl.sql.conn.GenericLanguageConnectionContext.popStatementContext(GenericLanguageConnectionContext.java:2286)
      at org.apache.derby.impl.jdbc.EmbedResultSet.updateRow(EmbedResultSet.java:3740)
      at org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug.testReproduction(ReproHoldCursorBug.java:71)
      at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
      at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
      at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
      at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:105)
      at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
      at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
      at junit.extensions.TestSetup.run(TestSetup.java:25)
      at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
      -----------------------------------8<--------------------------------------------
      (It shows a stack trace after this, but I'm attaching the result folders since those are more thorough.)[1]

      On the client driver, the JVM does not crash but it also errors out:
      1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)java.sql.SQLException: DERBY SQL error: SQLCODE: -1, SQLSTATE: XJ001, SQLERRMC: java.lang.NullPointerExceptionXJ001.U
      (more detail on the log files)[2]

      The error does not show as long as the "OF DATA" isn't specified. It also won't show if the whole FOR UPDATE clause is omitted.

      I would also like some comments and advice on how to proceed on converting holdCursorJDBC30.sql as it is affected by this problem. The original test does an update on a cursor with the "FOR UPDATE OF DATA" clause, and this isn't working on the Java version of the test. Do I go around it by removing the FOR UPDATE clause or should I wait for this bug to get fixed?

      Attachments:
      [1] - ErrorOutput_Embedded.tar.gz - Error output files of the Embedded run
      [2] - ErrorOutput_Client.tar.gz - Error output files of the Client/Server run
      [3] - ReproHoldCursorBug.java - The reproduction of the errors

      1. ErrorOutput_Client.tar.gz
        149 kB
        Tiago R. Espinha
      2. ErrorOutput_Embedded.tar.gz
        150 kB
        Tiago R. Espinha
      3. ReproHoldCursorBug.java
        3 kB
        Tiago R. Espinha
      4. derby-4198-throwable.diff
        1 kB
        Dag H. Wanvik
      5. derby-4198-throwable.stat
        0.1 kB
        Dag H. Wanvik
      6. derby-4198-1.diff
        26 kB
        Dag H. Wanvik
      7. derby-4198-1.stat
        1.0 kB
        Dag H. Wanvik
      8. derby-4198-1.diff
        25 kB
        Dag H. Wanvik
      9. derby-4198-1.stat
        0.9 kB
        Dag H. Wanvik
      10. derby-4198-2.diff
        24 kB
        Dag H. Wanvik
      11. derby-4198-2.stat
        0.9 kB
        Dag H. Wanvik
      12. derby-4198-3.stat
        0.9 kB
        Dag H. Wanvik
      13. derby-4198-3.diff
        24 kB
        Dag H. Wanvik
      14. derby-4198-4.diff
        21 kB
        Dag H. Wanvik
      15. derby-4198-4.stat
        0.9 kB
        Dag H. Wanvik

        Issue Links

          Activity

          Hide
          Dag H. Wanvik added a comment -

          There are actually two errors in this issue.

          The first is the underlying error which has this stack trace (disguised by error 2, see below):

          Error 1
          -------

          1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)java.sql.SQLException: Java exception: ': java.lang.NullPointerException'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:95)
          at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:87)
          at org.apache.derby.impl.jdbc.Util.javaException(Util.java:244)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403)
          at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346)
          at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2201)
          at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81)
          at org.apache.derby.impl.jdbc.EmbedResultSet.updateRow(EmbedResultSet.java:3743)
          at org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug.testReproduction(ReproHoldCursorBug.java:78)
          at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
          at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
          at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
          at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:105)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)
          at junit.extensions.TestSetup$1.protect(TestSetup.java:21)
          at junit.extensions.TestSetup.run(TestSetup.java:25)
          at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57)
          Caused by: java.sql.SQLException: Java exception: ': java.lang.NullPointerException'.
          at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119)
          at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70)
          ... 36 more
          Caused by: java.lang.NullPointerException
          at org.apache.derby.iapi.store.access.BackingStoreHashtable.getEstimatedMemUsage(BackingStoreHashtable.java:533)
          at org.apache.derby.iapi.store.access.BackingStoreHashtable.spillToDisk(BackingStoreHashtable.java:481)
          at org.apache.derby.iapi.store.access.BackingStoreHashtable.add_row_to_hash_table(BackingStoreHashtable.java:403)
          at org.apache.derby.iapi.store.access.BackingStoreHashtable.putRow(BackingStoreHashtable.java:731)
          at org.apache.derby.impl.sql.execute.ScrollInsensitiveResultSet.addRowToHashTable(ScrollInsensitiveResultSet.java:992)
          at org.apache.derby.impl.sql.execute.ScrollInsensitiveResultSet.updateRow(ScrollInsensitiveResultSet.java:1125)
          at org.apache.derby.impl.sql.execute.CurrentOfResultSet.updateRow(CurrentOfResultSet.java:334)
          at org.apache.derby.impl.sql.execute.ProjectRestrictResultSet.updateRow(ProjectRestrictResultSet.java:587)
          at org.apache.derby.impl.sql.execute.NormalizeResultSet.updateRow(NormalizeResultSet.java:407)
          at org.apache.derby.impl.sql.execute.UpdateResultSet.collectAffectedRows(UpdateResultSet.java:560)
          at org.apache.derby.impl.sql.execute.UpdateResultSet.open(UpdateResultSet.java:254)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:416)
          at org.apache.derby.impl.sql.GenericPreparedStatement.executeSubStatement(GenericPreparedStatement.java:286)
          at org.apache.derby.impl.jdbc.EmbedResultSet.updateRow(EmbedResultSet.java:3722)
          ... 29 more

          Error 2
          -------
          The error handling in EmbedResultSet.updateRow performs a redundant
          call to popStatementContext causing the assert error seen.
          There is a missing catch of Throwable at the end:
          :
          } catch (Throwable t)

          { throw handleException(t); }

          finally {
          :
          }
          This handling is lacking from insertRow and deleteRow as well.

          Show
          Dag H. Wanvik added a comment - There are actually two errors in this issue. The first is the underlying error which has this stack trace (disguised by error 2, see below): Error 1 ------- 1) testReproduction(org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug)java.sql.SQLException: Java exception: ': java.lang.NullPointerException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:95) at org.apache.derby.impl.jdbc.Util.newEmbedSQLException(Util.java:87) at org.apache.derby.impl.jdbc.Util.javaException(Util.java:244) at org.apache.derby.impl.jdbc.TransactionResourceImpl.wrapInSQLException(TransactionResourceImpl.java:403) at org.apache.derby.impl.jdbc.TransactionResourceImpl.handleException(TransactionResourceImpl.java:346) at org.apache.derby.impl.jdbc.EmbedConnection.handleException(EmbedConnection.java:2201) at org.apache.derby.impl.jdbc.ConnectionChild.handleException(ConnectionChild.java:81) at org.apache.derby.impl.jdbc.EmbedResultSet.updateRow(EmbedResultSet.java:3743) at org.apache.derbyTesting.functionTests.tests.store.ReproHoldCursorBug.testReproduction(ReproHoldCursorBug.java:78) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at org.apache.derbyTesting.junit.BaseTestCase.runBare(BaseTestCase.java:105) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24) at junit.extensions.TestSetup$1.protect(TestSetup.java:21) at junit.extensions.TestSetup.run(TestSetup.java:25) at org.apache.derbyTesting.junit.BaseTestSetup.run(BaseTestSetup.java:57) Caused by: java.sql.SQLException: Java exception: ': java.lang.NullPointerException'. at org.apache.derby.impl.jdbc.SQLExceptionFactory.getSQLException(SQLExceptionFactory.java:45) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.wrapArgsForTransportAcrossDRDA(SQLExceptionFactory40.java:119) at org.apache.derby.impl.jdbc.SQLExceptionFactory40.getSQLException(SQLExceptionFactory40.java:70) ... 36 more Caused by: java.lang.NullPointerException at org.apache.derby.iapi.store.access.BackingStoreHashtable.getEstimatedMemUsage(BackingStoreHashtable.java:533) at org.apache.derby.iapi.store.access.BackingStoreHashtable.spillToDisk(BackingStoreHashtable.java:481) at org.apache.derby.iapi.store.access.BackingStoreHashtable.add_row_to_hash_table(BackingStoreHashtable.java:403) at org.apache.derby.iapi.store.access.BackingStoreHashtable.putRow(BackingStoreHashtable.java:731) at org.apache.derby.impl.sql.execute.ScrollInsensitiveResultSet.addRowToHashTable(ScrollInsensitiveResultSet.java:992) at org.apache.derby.impl.sql.execute.ScrollInsensitiveResultSet.updateRow(ScrollInsensitiveResultSet.java:1125) at org.apache.derby.impl.sql.execute.CurrentOfResultSet.updateRow(CurrentOfResultSet.java:334) at org.apache.derby.impl.sql.execute.ProjectRestrictResultSet.updateRow(ProjectRestrictResultSet.java:587) at org.apache.derby.impl.sql.execute.NormalizeResultSet.updateRow(NormalizeResultSet.java:407) at org.apache.derby.impl.sql.execute.UpdateResultSet.collectAffectedRows(UpdateResultSet.java:560) at org.apache.derby.impl.sql.execute.UpdateResultSet.open(UpdateResultSet.java:254) at org.apache.derby.impl.sql.GenericPreparedStatement.executeStmt(GenericPreparedStatement.java:416) at org.apache.derby.impl.sql.GenericPreparedStatement.executeSubStatement(GenericPreparedStatement.java:286) at org.apache.derby.impl.jdbc.EmbedResultSet.updateRow(EmbedResultSet.java:3722) ... 29 more Error 2 ------- The error handling in EmbedResultSet.updateRow performs a redundant call to popStatementContext causing the assert error seen. There is a missing catch of Throwable at the end: : } catch (Throwable t) { throw handleException(t); } finally { : } This handling is lacking from insertRow and deleteRow as well.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a patch to fix the lacking error handling in updateRow, insertRow and deleteRow.
          This addresses the second error. Running regressions.

          Show
          Dag H. Wanvik added a comment - Uploading a patch to fix the lacking error handling in updateRow, insertRow and deleteRow. This addresses the second error. Running regressions.
          Hide
          Dag H. Wanvik added a comment -

          Given CREATE TABLE mytab (id int primary key, name varchar(50)), some
          failure modes and observations:

          a) rs = s.executeQuery("SELECT name from mytab for update of name");
          rs.next();
          rs.updateString(1,"foobar");

          fails because sourceRow of
          ScrollInsensitiveResultSet.addRowToHashTable is null, because
          ScrollInsensitiveResultSet.updateRow calls:

          prRS.doBaseRowProjection(row);

          but here the passed down row (UpdateResultSet.newBaseRow) is only 1
          long, so projection as done in the SELECT (projecting "name" out of
          the underlying base tables columns; which is what
          prRS.doBaseRowProjection does) is wrong.

          But projection is needed (see case f) if one does not use FOR
          UPDATE OF <col> since then one gets handed down a row whose # of
          columns is that of the basetable, and the hash table only wants the

          1. of rows selected, i.e. a projection is necessary.

          This one runs correct if the call to prRS.doBaseRowProjection(row)
          is removed.

          b) rs = s.executeQuery("SELECT id from mytab for update of id");
          rs.next();
          rs.updateString(1,20);

          works OK, but only accidentally, since projection of col 1 is a
          no-op.

          c) rs = s.executeQuery("SELECT * from mytab for update of id");
          rs.next();
          rs.updateInt(1,20);

          fails with empty pointer since hashRowArray is now one column
          longer (and the vacant position left in hash table is never set;
          the row passed down is only 1 long (it does not contain a value for
          column 2)

          d) rs = s.executeQuery("SELECT * from mytab for update of name");
          rs.next();
          rs.updateInt(2,"foobar");

          fails similarly to c, but in addition, column 2's ("name") new value
          is now set in the position of column 1 in the hash table.

          e) rs = s.executeQuery("SELECT * from mytab for update of id, name");
          rs.next();
          rs.updateInt(1, 20);
          rs.updateString(2,"foobar");
          rs.updateRow();

          works OK, since now both columns are updated.

          f) rs = s.executeQuery("SELECT * from mytab for update of id, name");
          rs.next();
          rs.updateInt(1, 20);
          // rs.updateString(2,"foobar");
          rs.updateRow();

          again fails.

          g) If we remove prRS.doBaseRowProjection(row); this query(for example)
          fails with arrayOutOfBounds

          rs = s.executeQuery("SELECT id from mytab for update");
          rs.next();
          rs.updateInt(1,20);
          rs.updateRow();

          because passed down row now has 2 columns, whereas hashRowArray
          only has space for one.

          h) rs = s.executeQuery("SELECT id from mytab for update of id, name");
          String cursorname = rs.getCursorName();
          rs.next();
          con.createStatement().executeUpdate("update mytab set name='foobar' where current of " +
          cursorname);

          This case should not touch hash table at all since it does not
          contain the "name" column. In fact, it seems to work, but write the "name"
          column to the "id" column in the hash table, which gives type error
          when trying to read it later.

          i) rs = s.executeQuery("SELECT id from mytab for update");
          String cursorname = rs.getCursorName();
          rs.next();
          con.createStatement().executeUpdate("update mytab set name='foobar' where current of " +
          cursorname);

          This works correctly because the passed down row has two columns, and
          ScrollInsensitiveResultSet the updated col 2 ("name"), effectively updating
          column 1 ("id") with its existing value.

          For update of the baserow, UpdateResultSet.collectAffectedRows (and
          updateDeferredRows) uses the object RowChanger:

          :
          source.updateRow(newBaseRow); – updates hash table
          rowChanger.updateRow(row, newBaseRow, baseRowLocation); – updates base row
          :

          which contains info on row column mappings between base table, select
          list and and columns to be updated. I think ScrollInsensitiveResultSet
          needs similar (but not identical) information: The row to be updated
          in the hash table has a (sub)set of the columns of the basetable
          determined by the SELECT list.

          The naming is a bit misleading, too, because "newBaseRow" has
          cardinality 1 in the cases where one specifies FOR UPDATE OF
          <col-list> and updates only one column, no matter how many columns
          are specified in <col-list>. So, even mentioning all columns leads to
          a different code path that omitting the OF <col-list>. Omitting it,
          makes newBaseRow always ahve the cardinality of the underlying base
          table, as the name would suggest.

          Show
          Dag H. Wanvik added a comment - Given CREATE TABLE mytab (id int primary key, name varchar(50)), some failure modes and observations: a) rs = s.executeQuery("SELECT name from mytab for update of name"); rs.next(); rs.updateString(1,"foobar"); fails because sourceRow of ScrollInsensitiveResultSet.addRowToHashTable is null, because ScrollInsensitiveResultSet.updateRow calls: prRS.doBaseRowProjection(row); but here the passed down row (UpdateResultSet.newBaseRow) is only 1 long, so projection as done in the SELECT (projecting "name" out of the underlying base tables columns; which is what prRS.doBaseRowProjection does) is wrong. But projection is needed (see case f) if one does not use FOR UPDATE OF <col> since then one gets handed down a row whose # of columns is that of the basetable, and the hash table only wants the of rows selected, i.e. a projection is necessary. This one runs correct if the call to prRS.doBaseRowProjection(row) is removed. b) rs = s.executeQuery("SELECT id from mytab for update of id"); rs.next(); rs.updateString(1,20); works OK, but only accidentally, since projection of col 1 is a no-op. c) rs = s.executeQuery("SELECT * from mytab for update of id"); rs.next(); rs.updateInt(1,20); fails with empty pointer since hashRowArray is now one column longer (and the vacant position left in hash table is never set; the row passed down is only 1 long (it does not contain a value for column 2) d) rs = s.executeQuery("SELECT * from mytab for update of name"); rs.next(); rs.updateInt(2,"foobar"); fails similarly to c, but in addition, column 2's ("name") new value is now set in the position of column 1 in the hash table. e) rs = s.executeQuery("SELECT * from mytab for update of id, name"); rs.next(); rs.updateInt(1, 20); rs.updateString(2,"foobar"); rs.updateRow(); works OK, since now both columns are updated. f) rs = s.executeQuery("SELECT * from mytab for update of id, name"); rs.next(); rs.updateInt(1, 20); // rs.updateString(2,"foobar"); rs.updateRow(); again fails. g) If we remove prRS.doBaseRowProjection(row); this query(for example) fails with arrayOutOfBounds rs = s.executeQuery("SELECT id from mytab for update"); rs.next(); rs.updateInt(1,20); rs.updateRow(); because passed down row now has 2 columns, whereas hashRowArray only has space for one. h) rs = s.executeQuery("SELECT id from mytab for update of id, name"); String cursorname = rs.getCursorName(); rs.next(); con.createStatement().executeUpdate("update mytab set name='foobar' where current of " + cursorname); This case should not touch hash table at all since it does not contain the "name" column. In fact, it seems to work, but write the "name" column to the "id" column in the hash table, which gives type error when trying to read it later. i) rs = s.executeQuery("SELECT id from mytab for update"); String cursorname = rs.getCursorName(); rs.next(); con.createStatement().executeUpdate("update mytab set name='foobar' where current of " + cursorname); This works correctly because the passed down row has two columns, and ScrollInsensitiveResultSet the updated col 2 ("name"), effectively updating column 1 ("id") with its existing value. For update of the baserow, UpdateResultSet.collectAffectedRows (and updateDeferredRows) uses the object RowChanger: : source.updateRow(newBaseRow); – updates hash table rowChanger.updateRow(row, newBaseRow, baseRowLocation); – updates base row : which contains info on row column mappings between base table, select list and and columns to be updated. I think ScrollInsensitiveResultSet needs similar (but not identical) information: The row to be updated in the hash table has a (sub)set of the columns of the basetable determined by the SELECT list. The naming is a bit misleading, too, because "newBaseRow" has cardinality 1 in the cases where one specifies FOR UPDATE OF <col-list> and updates only one column, no matter how many columns are specified in <col-list>. So, even mentioning all columns leads to a different code path that omitting the OF <col-list>. Omitting it, makes newBaseRow always ahve the cardinality of the underlying base table, as the name would suggest.
          Hide
          Dag H. Wanvik added a comment -

          Uploading a first patch for this issue. It passes the RowChanger
          object on down to ScrollInsensitiveResultSet, so that it can do the
          proper projections itself. Any underlying PRN is no longer called upon
          to do the project, rather this now happens in
          ScrollInsensitiveResultSet.update itself.

          Unfortunately this "opens up" RowChangerImpl logic a bit by
          introducing two new public methods to get at its internal structures,
          feel free to suggest a better way to encapsulate this logic. I guess I
          could move the method ScrollInsensitiveResultSet.findSelectedCol
          inside RowChanger...

          SURTest.java has a new test fixture with several scenarios that fail
          (or succeed by "accident") without the patch.

          I also logged DERBY-4226 as part of this investigation. SURTest
          contains a scenario that will fail when that bug is fixed; it is
          included here because the patch touches on that logic (a base column
          selected more than once in a updatable result set) as well.

          The patch also contains a temporary (I intend to remove it before
          commit) debug flag for some diagnostic output which may help a
          reviewer to understand what's going on. Enable with
          -Dderby.debug.true=SURMapping.

          I did run regressions OK with an earlier version of this patch. Rerunning again now.

          Show
          Dag H. Wanvik added a comment - Uploading a first patch for this issue. It passes the RowChanger object on down to ScrollInsensitiveResultSet, so that it can do the proper projections itself. Any underlying PRN is no longer called upon to do the project, rather this now happens in ScrollInsensitiveResultSet.update itself. Unfortunately this "opens up" RowChangerImpl logic a bit by introducing two new public methods to get at its internal structures, feel free to suggest a better way to encapsulate this logic. I guess I could move the method ScrollInsensitiveResultSet.findSelectedCol inside RowChanger... SURTest.java has a new test fixture with several scenarios that fail (or succeed by "accident") without the patch. I also logged DERBY-4226 as part of this investigation. SURTest contains a scenario that will fail when that bug is fixed; it is included here because the patch touches on that logic (a base column selected more than once in a updatable result set) as well. The patch also contains a temporary (I intend to remove it before commit) debug flag for some diagnostic output which may help a reviewer to understand what's going on. Enable with -Dderby.debug.true=SURMapping. I did run regressions OK with an earlier version of this patch. Rerunning again now.
          Hide
          Dag H. Wanvik added a comment -

          Patch derby-4198-throwable committed as svn 774148.

          Show
          Dag H. Wanvik added a comment - Patch derby-4198-throwable committed as svn 774148.
          Hide
          Dag H. Wanvik added a comment - - edited

          Uploading 2nd spin, derby-4198-2 of the patch for the first problem of this issue.
          This moves the logic of findSelectedCol to inside RowChanger for better encapsulation
          of that logic. I also removed a vacuous (I believe) call to source.updateRow
          from the deferred update code of UpdateResultSet. That code is only ever run as
          part of a referential action and is not relevant for updating an scrollable insensitive result set.

          Ready for review.

          Show
          Dag H. Wanvik added a comment - - edited Uploading 2nd spin, derby-4198-2 of the patch for the first problem of this issue. This moves the logic of findSelectedCol to inside RowChanger for better encapsulation of that logic. I also removed a vacuous (I believe) call to source.updateRow from the deferred update code of UpdateResultSet. That code is only ever run as part of a referential action and is not relevant for updating an scrollable insensitive result set. Ready for review.
          Hide
          Dag H. Wanvik added a comment -

          Uploading version #3 of this patch; just improved comments.
          I plan to commit this patch shortly.

          Show
          Dag H. Wanvik added a comment - Uploading version #3 of this patch; just improved comments. I plan to commit this patch shortly.
          Hide
          Knut Anders Hatlen added a comment -

          Hi Dag,

          I had a quick look at the #3 patch and also did some manual testing of
          it. It basically looks good. Some questions and very minor nits:

          • The javadoc for FormatableBitSet.anySetBit(int) is not very clear on
            the behaviour when beyondBit is negative. As far as I can see from
            the implementation, -1 is OK but other negative values will cause
            ArrayIndexOutOfBoundsException. Do you think it would make sense to
            add a sentence to its javadoc stating that beyondBit=-1 can be used
            to search from the start of the bit set? Then it would be quite clear
            that the usage in findSelectedCol() is OK.
          • In ScrollInsensitiveResultSet.updateRow(), it's not necessary to
            initialize map to null. By not initializing it we gain a
            compile-time check that the array is never used uninitialized
            instead of getting an NPE at run-time. (There's no problem now, but
            a compile-time check will reduce the risk of introducing bugs if the
            code is changed later.)
          • RowChangerImpl.toString() returns "" in non-debug builds. Would
            super.toString() be better?
          • You mentioned in an earlier comment that you would remove the debug
            code before commit. Is that still the plan?
          Show
          Knut Anders Hatlen added a comment - Hi Dag, I had a quick look at the #3 patch and also did some manual testing of it. It basically looks good. Some questions and very minor nits: The javadoc for FormatableBitSet.anySetBit(int) is not very clear on the behaviour when beyondBit is negative. As far as I can see from the implementation, -1 is OK but other negative values will cause ArrayIndexOutOfBoundsException. Do you think it would make sense to add a sentence to its javadoc stating that beyondBit=-1 can be used to search from the start of the bit set? Then it would be quite clear that the usage in findSelectedCol() is OK. In ScrollInsensitiveResultSet.updateRow(), it's not necessary to initialize map to null. By not initializing it we gain a compile-time check that the array is never used uninitialized instead of getting an NPE at run-time. (There's no problem now, but a compile-time check will reduce the risk of introducing bugs if the code is changed later.) RowChangerImpl.toString() returns "" in non-debug builds. Would super.toString() be better? You mentioned in an earlier comment that you would remove the debug code before commit. Is that still the plan?
          Hide
          Dag H. Wanvik added a comment -

          Thanks for the review, Knut.
          Uploading patch derby-4198-4 incorporating your comments.
          I added a sentence to FormatableBitSet:

          • @param beyondBit Only look at bit that is greater than this bit number.
          • Supplying a value of -1 makes the call equivalent to
          • anySetBit().
          Show
          Dag H. Wanvik added a comment - Thanks for the review, Knut. Uploading patch derby-4198-4 incorporating your comments. I added a sentence to FormatableBitSet: @param beyondBit Only look at bit that is greater than this bit number. Supplying a value of -1 makes the call equivalent to anySetBit().
          Hide
          Dag H. Wanvik added a comment -

          Committed derby-4198-4 on trunk as svn 785163.

          Show
          Dag H. Wanvik added a comment - Committed derby-4198-4 on trunk as svn 785163.
          Hide
          Dag H. Wanvik added a comment -

          Backported to 10.5 branch as svn 785165.

          Show
          Dag H. Wanvik added a comment - Backported to 10.5 branch as svn 785165.
          Hide
          Kathey Marsden added a comment -

          There is a new javadoc warning on trunk and 10.5:
          [javadoc] C:\nightlies\main\src\opensource\java\engine\org\apache\derby\iapi\sql\execute\RowChanger.java:175: warning - @returns is an unknown tag.
          [javadoc] 1 warning

          which may be related to this change.

          Show
          Kathey Marsden added a comment - There is a new javadoc warning on trunk and 10.5: [javadoc] C:\nightlies\main\src\opensource\java\engine\org\apache\derby\iapi\sql\execute\RowChanger.java:175: warning - @returns is an unknown tag. [javadoc] 1 warning which may be related to this change.
          Hide
          Kristian Waagan added a comment -

          I took the liberty of fixing this to test the Hudson CI build trigger.
          Committed to trunk with revision 786078 and backported to 10.5 with revision 786080.

          Show
          Kristian Waagan added a comment - I took the liberty of fixing this to test the Hudson CI build trigger. Committed to trunk with revision 786078 and backported to 10.5 with revision 786080.
          Hide
          Dag H. Wanvik added a comment -

          Tiago, can you close this issue?

          Show
          Dag H. Wanvik added a comment - Tiago, can you close this issue?

            People

            • Assignee:
              Dag H. Wanvik
              Reporter:
              Tiago R. Espinha
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development