Derby
  1. Derby
  2. DERBY-5425

Updateable holdable ResultSet terminates early after 65638 updates

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 10.7.1.1
    • Fix Version/s: 10.8.3.0, 10.9.2.2, 10.10.1.1
    • Component/s: JDBC
    • Environment:
    • Urgency:
      Normal
    • Issue & fix info:
      Repro attached
    • Bug behavior facts:
      Seen in production, Wrong query result

      Description

      After at least 65638 updates to an indexed column have been done via an updateable holdable resultset and the transaction is committed ResultSet.next() returns false even if more rows exist to be returned.

      The following program should output "Total: 100000" but instead outputs "Total: 65638".

      import java.sql.Connection;
      import java.sql.DriverManager;
      import java.sql.PreparedStatement;
      import java.sql.ResultSet;
      import java.sql.SQLException;
      import java.sql.Statement;

      public class DerbyBug {
      public static void main(String[] args) throws ClassNotFoundException, SQLException {
      Class.forName("org.apache.derby.jdbc.EmbeddedDriver");
      Connection conn = DriverManager.getConnection("jdbc:derby:TestDB;create=true");

      conn.setAutoCommit(false);

      Statement createStmt = conn.createStatement();
      createStmt.executeUpdate("CREATE TABLE test (a INT)");
      createStmt.executeUpdate("CREATE INDEX idxa ON test(a)");
      createStmt.close();

      PreparedStatement insertStmt = conn.prepareStatement("INSERT INTO test(a) VALUES ");

      for (int i = 0; i < 100000; ++i)

      { insertStmt.setInt(1, i); insertStmt.executeUpdate(); }

      insertStmt.close();

      conn.commit();

      Statement selectStmt = conn.createStatement(ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_UPDATABLE, ResultSet.HOLD_CURSORS_OVER_COMMIT);
      ResultSet rs = selectStmt.executeQuery("SELECT a FROM test FOR UPDATE");

      int count = 0;
      while (rs.next())

      { rs.updateInt(1, count); rs.updateRow(); count++; conn.commit(); }

      rs.close();
      selectStmt.close();
      conn.commit();
      conn.close();

      System.out.println("Total: " + count);

      try

      { DriverManager.getConnection("jdbc:derby:;shutdown=true"); }

      catch (SQLException e) {
      if (!e.getSQLState().equals("XJ015"))

      { throw e; }

      }
      }
      }

      1. d5425-1a.diff.txt
        23 kB
        Knut Anders Hatlen
      2. holdable-row-holders.diff.txt
        3 kB
        Knut Anders Hatlen
      3. DerbyBug.java
        2 kB
        Andrew Johnson

        Issue Links

          Activity

          Hide
          Andrew Johnson added a comment -

          Reproduction

          Show
          Andrew Johnson added a comment - Reproduction
          Hide
          Knut Anders Hatlen added a comment -

          Reproduced on trunk. Some observations:

          When an updatable result set uses an index, and a value is changed so that the row may be found again later in the index scan (typically because the key value is increased), the row location is stored in a hashtable to make it easy to skip that row when it's seen again. When the size of the hashtable exceeds 1/16 of derby.language.maxMemoryPerTable (default: 1048576/16=65536 rows), a TemporaryRowHolder is created to hold the overflowing rows. The TRH spills to disk when its size exceeds 100 rows. That is, it spills to disk after 65536+100=65636 row locations have been stored, which is very close to the number of rows seen by the repro.

          If UpdateResultSet.notifyForUpdateCursor() is modified to use the hashtable no matter how large it is, and never use the TemporaryRowHolder, the repro returns the expected number of rows (100000).

          If UpdateResultSet.notifyForUpdateCursor() is modified to create a TemporaryRowHolder instance that holds more than 100 rows before it spills to disk, the number of rows returned by the repro increases.

          Increasing derby.language.maxMemoryPerTable also makes the repro return more rows.

          So it looks like the problem is somehow related to this overflow handling.

          I also noticed that removing the commit() call after each row update in the repro, all rows were returned. At the same time I noticed that the temporary conglomerate created by TemporaryRowHolder is not holdable over commits (see second argument to openConglomerate() call in TemporaryRowHolderImpl.insert()), which sounds suspicious. However, simply hard-coding TemporaryRowHolderImpl.insert() to create conglomerates that are holdable over commit, didn't change the number of rows returned.

          Show
          Knut Anders Hatlen added a comment - Reproduced on trunk. Some observations: When an updatable result set uses an index, and a value is changed so that the row may be found again later in the index scan (typically because the key value is increased), the row location is stored in a hashtable to make it easy to skip that row when it's seen again. When the size of the hashtable exceeds 1/16 of derby.language.maxMemoryPerTable (default: 1048576/16=65536 rows), a TemporaryRowHolder is created to hold the overflowing rows. The TRH spills to disk when its size exceeds 100 rows. That is, it spills to disk after 65536+100=65636 row locations have been stored, which is very close to the number of rows seen by the repro. If UpdateResultSet.notifyForUpdateCursor() is modified to use the hashtable no matter how large it is, and never use the TemporaryRowHolder, the repro returns the expected number of rows (100000). If UpdateResultSet.notifyForUpdateCursor() is modified to create a TemporaryRowHolder instance that holds more than 100 rows before it spills to disk, the number of rows returned by the repro increases. Increasing derby.language.maxMemoryPerTable also makes the repro return more rows. So it looks like the problem is somehow related to this overflow handling. I also noticed that removing the commit() call after each row update in the repro, all rows were returned. At the same time I noticed that the temporary conglomerate created by TemporaryRowHolder is not holdable over commits (see second argument to openConglomerate() call in TemporaryRowHolderImpl.insert()), which sounds suspicious. However, simply hard-coding TemporaryRowHolderImpl.insert() to create conglomerates that are holdable over commit, didn't change the number of rows returned.
          Hide
          Knut Anders Hatlen added a comment -

          After some more debugging, it turned out this issue happens because the temporary conglomerates are not holdable over commits, as suggested above. When the temporary row holder spills to disk, all rows are written to it, but they are lost on commit, so that the scan is emptied and too few rows are returned.

          I mentioned in my previous comment that I had tried making TemporaryRowHolder's conglomerate holdable over commit with no luck. However, with the attached patch (holdable-row-holders.diff.txt) the repro successfully returns 100000 rows. That patch also makes the scans in TemporaryRowHolderResultSet holdable.

          This patch is not intended for commit, since it unconditionally changes the holdability of all temporary row holders, but at least it shows that the lack of holdability is what's causing this bug.

          Show
          Knut Anders Hatlen added a comment - After some more debugging, it turned out this issue happens because the temporary conglomerates are not holdable over commits, as suggested above. When the temporary row holder spills to disk, all rows are written to it, but they are lost on commit, so that the scan is emptied and too few rows are returned. I mentioned in my previous comment that I had tried making TemporaryRowHolder's conglomerate holdable over commit with no luck. However, with the attached patch (holdable-row-holders.diff.txt) the repro successfully returns 100000 rows. That patch also makes the scans in TemporaryRowHolderResultSet holdable. This patch is not intended for commit, since it unconditionally changes the holdability of all temporary row holders, but at least it shows that the lack of holdability is what's causing this bug.
          Hide
          Knut Anders Hatlen added a comment -

          The problematic code implements essentially the same functionality as BackingStoreHashtable. If we could reuse BackingStoreHashtable here, it might fix the bug and probably also reduce the amount of code.

          Show
          Knut Anders Hatlen added a comment - The problematic code implements essentially the same functionality as BackingStoreHashtable. If we could reuse BackingStoreHashtable here, it might fix the bug and probably also reduce the amount of code.
          Hide
          Knut Anders Hatlen added a comment -

          I'm uploading a patch (d5425-1a.diff.txt) that makes updatable cursors use BackingStoreHashtable to keep track of the rows that have been moved in the scan direction and therefore need to be ignored the next time they are seen. BackingStoreHashtable has a flag that specifies whether its contents should be kept over commit, which makes it possible to get around the problems with the original code.

          In essence, the patch just makes UpdateResultSet.notifyForUpdateCursor() create a BackingStoreHashtable instead of a java.util.Hashtable, and removes all the code that handles overflow to a TemporaryRowHolder instance as that's now handled transparently by BackingStoreHashtable.

          One existing test case had to be modified to pass with the patch. UpdateCursorTest.testVirtualMemoryHeap() expected a particular ordering of the rows. Specifically, it expected the rows stored in the in-memory portion of the TemporaryRowHolder to come out in reversed order. Since we no longer use a TemporaryRowHolder to store the overflow rows when the patch is applied, the rows returned from the index scan will follow the index ordering also when the hash table overflows. I would think the new ordering is more in line with the users' expectation of how results from an index scan are ordered.

          All the regression tests ran cleanly with the patch.

          More detailed description of the changes:

          • impl/sql/execute/UpdateResultSet.java

          Create BackingStoreHashtable instead of java.util.Hashtable.

          • Initialize BackingStoreHashtable.keepAfterCommit based on the holdability of the underlying scan.
          • Use same initial capacity and maximum capacity as the original hash table, so that the memory footprint will be roughly the same.

          Remove code that handles the hash table growing beyond maxCapacity, as that's handled internally in BackingStoreHashtable now by spilling to disk.

          • impl/sql/execute/TableScanResultSet.java

          Remove code that reads the overflow rows, as the distinction between rows in memory and rows spilt to disk .

          Remove fields that are only used to handle overflow rows, and code that checks the values of those fields.

          Make close() release the resources held by the BackingStoreHashtable.

          • impl/sql/execute/IndexRowToBaseRowResultSet.java

          Remove fields that are only used to handle overflow rows

          • impl/sql/execute/CurrentOfResultSet.java

          Remove code that reads the overflow rows, and the fields that are only used in the removed code.

          • functionTests/tests/lang/UpdateCursorTest.java

          Update expected ordering of rows in testVirtualMemoryHeap() as described above.

          Add test case testDerby5425HoldOverCommit() which exposes this bug and verifies the fix. Since UpdateCursorTest sets derby.language.maxMemoryPerTable to 1, the bug can be exposed with a much smaller number of rows than in the original repro.

          Show
          Knut Anders Hatlen added a comment - I'm uploading a patch (d5425-1a.diff.txt) that makes updatable cursors use BackingStoreHashtable to keep track of the rows that have been moved in the scan direction and therefore need to be ignored the next time they are seen. BackingStoreHashtable has a flag that specifies whether its contents should be kept over commit, which makes it possible to get around the problems with the original code. In essence, the patch just makes UpdateResultSet.notifyForUpdateCursor() create a BackingStoreHashtable instead of a java.util.Hashtable, and removes all the code that handles overflow to a TemporaryRowHolder instance as that's now handled transparently by BackingStoreHashtable. One existing test case had to be modified to pass with the patch. UpdateCursorTest.testVirtualMemoryHeap() expected a particular ordering of the rows. Specifically, it expected the rows stored in the in-memory portion of the TemporaryRowHolder to come out in reversed order. Since we no longer use a TemporaryRowHolder to store the overflow rows when the patch is applied, the rows returned from the index scan will follow the index ordering also when the hash table overflows. I would think the new ordering is more in line with the users' expectation of how results from an index scan are ordered. All the regression tests ran cleanly with the patch. More detailed description of the changes: impl/sql/execute/UpdateResultSet.java Create BackingStoreHashtable instead of java.util.Hashtable. Initialize BackingStoreHashtable.keepAfterCommit based on the holdability of the underlying scan. Use same initial capacity and maximum capacity as the original hash table, so that the memory footprint will be roughly the same. Remove code that handles the hash table growing beyond maxCapacity, as that's handled internally in BackingStoreHashtable now by spilling to disk. impl/sql/execute/TableScanResultSet.java Remove code that reads the overflow rows, as the distinction between rows in memory and rows spilt to disk . Remove fields that are only used to handle overflow rows, and code that checks the values of those fields. Make close() release the resources held by the BackingStoreHashtable. impl/sql/execute/IndexRowToBaseRowResultSet.java Remove fields that are only used to handle overflow rows impl/sql/execute/CurrentOfResultSet.java Remove code that reads the overflow rows, and the fields that are only used in the removed code. functionTests/tests/lang/UpdateCursorTest.java Update expected ordering of rows in testVirtualMemoryHeap() as described above. Add test case testDerby5425HoldOverCommit() which exposes this bug and verifies the fix. Since UpdateCursorTest sets derby.language.maxMemoryPerTable to 1, the bug can be exposed with a much smaller number of rows than in the original repro.
          Hide
          Knut Anders Hatlen added a comment -

          Committed revision 1359052.

          Show
          Knut Anders Hatlen added a comment - Committed revision 1359052.
          Hide
          Kathey Marsden added a comment -

          Reopen for backport analysis. Temporarily assign to yourself if you backport and then reassign to Knut Anders before closing.

          Show
          Kathey Marsden added a comment - Reopen for backport analysis. Temporarily assign to yourself if you backport and then reassign to Knut Anders before closing.
          Hide
          Knut Anders Hatlen added a comment -

          The patch merged cleanly to 10.9 and 10.8. However, I'm seeing failures in the compatibility test on the 10.8 branch. I won't backport until I understand why it's failing.

          Show
          Knut Anders Hatlen added a comment - The patch merged cleanly to 10.9 and 10.8. However, I'm seeing failures in the compatibility test on the 10.8 branch. I won't backport until I understand why it's failing.
          Hide
          Knut Anders Hatlen added a comment -

          The compatibility tests failed the first two times I ran it on the 10.8 branch. It varied which combinations failed, and I couldn't see what caused it, only that some of the test cases failed with "database not found" errors. However, I've now re-run the compatibility tests many times without seeing the error again, so I'm assuming it was just a temporary environment problem and/or one of the instabilities in the tests' sub-process handling that we've cleaned up on trunk.

          Merged to 10.9 with revision 1387111.
          Merged to 10.8 with revision 1387112.

          Show
          Knut Anders Hatlen added a comment - The compatibility tests failed the first two times I ran it on the 10.8 branch. It varied which combinations failed, and I couldn't see what caused it, only that some of the test cases failed with "database not found" errors. However, I've now re-run the compatibility tests many times without seeing the error again, so I'm assuming it was just a temporary environment problem and/or one of the instabilities in the tests' sub-process handling that we've cleaned up on trunk. Merged to 10.9 with revision 1387111. Merged to 10.8 with revision 1387112.

            People

            • Assignee:
              Knut Anders Hatlen
              Reporter:
              Andrew Johnson
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development