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

• Key: DERBY-4198
• Type: Bug
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Dag H. Wanvik
• Reporter: Tiago R. Espinha
• Component/s: JDBC, Network Client, SQL
• 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.0.0

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

Comments

[Dag H. Wanvik]

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.

[Dag H. Wanvik]

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

[Dag H. Wanvik]

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.

[Dag H. Wanvik]

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.

[Dag H. Wanvik]

Patch derby-4198-throwable committed as svn 774148.

[Dag H. Wanvik]

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.

[Dag H. Wanvik]

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

[Knut Anders Hatlen]

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?

[Dag H. Wanvik]

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().

[Dag H. Wanvik]

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

[Dag H. Wanvik]

Backported to 10.5 branch as svn 785165.

[Kathey Marsden]

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.

[Kristian Waagan]

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.