Mailing-List: contact derby-dev-help@db.apache.org; run by ezmlm
Precedence: bulk
Reply-To: <derby-dev@db.apache.org>
Message-ID: <3362009.1177106716943.JavaMail.jira@brutus>
Date: Fri, 20 Apr 2007 15:05:16 -0700 (PDT)
From: "A B (JIRA)" <jira@apache.org>
To: derby-dev@db.apache.org
Subject: [jira] Commented: (DERBY-2462)
 org.apache.derby.impl.store.access.BackingStoreHashTableFromScan does not
 honor ResultSet holdability
In-Reply-To: <5766003.1174072989285.JavaMail.jira@brutus>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


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

A B commented on DERBY-2462:
----------------------------

I am not familiar with this area of code so I cannot make any definitive comments on whether or not the changes are correct nor on if there may be a better way.  I did look at the patch, though, and had the following two questions.  These are both minor and somewhat theoretical, so I don't think either should block commit of the patch.  Just my proverbial two cents...

1) The interaction of the various ScanController interfaces and implementations has me completely confused (which has nothing to do with your changes) but I did notice one interface called ScanManager, which extends ScanController, that defines a "closeForEndTransaction(boolean)" method:

  http://db.apache.org/derby/javadoc/engine/org/apache/derby/iapi/store/access/conglomerate/ScanManager.html

I also noticed that most of the Scan implementations apparently implement the ScanManager class.  At least, that's what I gather from the above javadoc page; I tried to verify that by looking at the actual code but got lost in no time.  In any event, I was wondering if that method could somehow be used to indicate that a ScanController has been closed as the result of a commit.  Then instead of catching an exception in DiskHashtable, you could try something like:

     try
     {
        // DERBY-2462: if holdable and scan got closed due
        // to commit, we need to reopen where we left off
        if (scan.closedByCommit() && keepAfterCommit)
        {
            scan = openRowConglomerate(tc, rowConglomerateId);
            scan.positionAtRowLocation(rowloc);
        }
        scan.fetch(row);
        
where "scan.closedByCommit()" would return true if the scan was closed by a ScanManager.closeForEndTransaction() call.

As I said, that's just a theoretical.  I don't know if it's actually possible to get something like that working, but since I noticed the method I thought I'd ask.

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?

2) The patch includes the following diff in DiskHashtable:

 +    // DERBY-2462: if holdable and scan got closed due
 +    // to commit, we need to reopen where we left off
 +    if (keepAfterCommit && (SQLState.AM_SCAN_NOT_POSITIONED.
 +            substring(0,5).
 +            equals(e.getSQLState()))) {
 +        scan = openRowConglomerate(tc, rowConglomerateId);
 +        scan.positionAtRowLocation(rowloc);

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?

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.

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.  If any committer out there (including you) wishes to commit these changes as they are, I would not complain.

On a more concrete level, I verified that the new lang/SpillHash.java test runs cleanly with your changes and fails with error XSCH6 without them, as expected.  So thanks for fixing that test up.

> 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, 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.