[jira] Updated: (DERBY-2462) org.apache.derby.impl.store.access.BackingStoreHashTableFromScan does not honor ResultSet holdability

["Dag H. Wanvik (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/DERBY-2462?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Dag H. Wanvik updated DERBY-2462:
---------------------------------

    Attachment: DERBY-2462-3.stat
                DERBY-2462-3.diff

Thanks for the review, Army! 

I attach a new version of the patch, DERBY-2462-3.* which supercedes
earlier versions. I address your comments explicitly below. Relative
to the previous version (*-2.diff), this patch:

1) opens the heap scan in DiskHashtable#ElementEnum's constructor with
   hold=true if we have holdability. Required for item 3) below to
   work.

2) avoids having to catch the not positioned exception in
   DiskHashtable#nextElement, when the scan has been closed, by using
   the new method ScanController#isPositioned.

3) removes the call to openRowConglomerate, now relying on
   positionAtRowLocation to do the reopening when required under
   holdability. 

4) checks the result of positionAtRowLocation to verify that it
   worked, else throw an exception "24000" NO_CURRENT_ROW. It can fail
   if the row location has been invalidated by compress. Pretty
   unlikely, but one never knows..

5) removes the exit() calls from the SpillHash.java test to make it
   runnable with -Duseprocess=false

6) some small cleanup in DiskHashtable.

Regression tests ran cleanly on Sun 1.4 Solaris 10/x86.


Answers to Army's review comments:

> 1) The interaction of the various ScanController interfaces and
> implementations has me completely confused (which has nothing to do

you are not alone... ;)

> If that type of check is not possible then it seems like we should
> at least be able to add an "isClosed()" or "isPositioned()" method
> to the ScanController that could be used instead of catching the
> exception. Is there something in particular that would make such an
> aproach infeasible?

Yes, I thought of something like this too. I finally added a method
ScanController#isPositioned, and implemented it for heap and btree
scans (only used by the former, though).

> It looks like we first make a call to reopen the scan, then we call
> "positionAtRowLocation()". But the javadoc for the latter method
> (see ScanController.java) seems to indicate that the reopen happens
> automatically:
> 
>     /**
>      * Positions the scan at row location and locks the row.
>      * If the scan is not opened, it will be reopened if this is a holdable
>      * scan and there has not been any operations which causes RowLocations
>      * to be invalidated.
> 
> Is this javadoc correct? If so, then is it necessary to explicitly
> call "openRowConglomerate()" in the above diff, or could we get by
> without it?

Yes, no and yes. Much cleaner, thanks. See item 1) and 3) above.

> Also, the javadoc mentions that "positionAtRowLocation()" locks the
> row. The javadoc for ScanController.fetch() doesn't mention anything
> about locking the row, but I'm assuming it happens anyway...is that
> correct? (excuse my ignorance here). Can you confirm that row
> locking occurs as expected with these changes? I have no reason to
> believe otherwise, just thought I'd ask.

Yes, locking is performed as a consequence of calling
positionAtRowLocation, see HeapScan#positionAtRowLocation which calls
reopenScanByRecordHandleAndSetLocks. 

> It's worth repeating here that I am not familiar with this area of
> code and thus this feedback could be of little-to-no-value, so
> please keep that in mind. 

I very much appreciate your feedback; I am also unfamiliar with this
area of the code, and an extra set of eyes is always useful! The patch
is cleaner now, I think. Thanks again! :)


> org.apache.derby.impl.store.access.BackingStoreHashTableFromScan does not honor ResultSet holdability
> -----------------------------------------------------------------------------------------------------
>
>                 Key: DERBY-2462
>                 URL: https://issues.apache.org/jira/browse/DERBY-2462
>             Project: Derby
>          Issue Type: Bug
>          Components: Store
>    Affects Versions: 10.1.1.0, 10.1.2.1, 10.1.3.1, 10.2.1.6, 10.2.2.0
>         Environment: Test under Windows Vista, Java 1.4.2_13
>            Reporter: Jeff Clary
>         Assigned To: Dag H. Wanvik
>         Attachments: DERBY-2462-1.diff, DERBY-2462-1.stat, DERBY-2462-2.diff, DERBY-2462-2.stat, DERBY-2462-3.diff, DERBY-2462-3.stat, DerbyHoldabilityTest.java
>
>
> After an unrelated statement on the same connection commits, and after some number of successful calls to ResultSet.next(), a subsequent call to ResultSet.next() throws an SQLException with a message like: The heap container with container id Container(-1, 1173965368428) is closed.  This seems to be related to the hard-coded passing of false to the super in the constructor of org.apache.derby.impl.store.access.BackingStoreHashTableFromScan.
> Steps to reproduce:
> 1. Execute a statement on a connection that returns a result set.
> 2. Execute a second statement on the same connection that modifies the database and commits.
> 3. Call next() on the first result set until the exception is thrown.
> Note that the number of rows that can be successfully retrieved from the result set seems to be related to the amount of data per row.  Increasing the number of columns in the result set or the length of the columns causes the exception to be taken sooner.
> The attached test program demonstrates the issue.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.