db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dag H. Wanvik (JIRA)" <j...@apache.org>
Subject [jira] Commented: (DERBY-2462) org.apache.derby.impl.store.access.BackingStoreHashTableFromScan does not honor ResultSet holdability
Date Tue, 24 Apr 2007 23:15:16 GMT

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

Dag H. Wanvik commented on DERBY-2462:

Thanks for your continued help with this one, Army!

> 1) ...
>   My question is: are there any scenarios in which we could get to
> the new code when the scan was closed for some reason *other* than a
> commit? If this is possible, will attempting to re-open the scan in
> such a scenario cause problems?

I don't believe so, but let me try an analysis:

The states of the scan (heap, I omit btree here for simplicity
although i believe it is similar) is one of


 Btw, the comment at the top of GenericScanController (used for heap
 scan along with subclass HeapScan) omits the *_HOLD_* states, so it
 is out of date..

* The constructor of DiskHashTable#ElementEnum opens the scan.  This
  moves the scan state to SCAN_INIT if successful. If not, it is
  silently swallowed (not good!), and the enumeration will have zero
  elements (hasMore==false).

* Next, the constructor performs a scan.next() which sets the boolean
  state variable "hasMore". When this is true, the scan state will be
  SCAN_INPROGRESS, when its is false, there are no more data and the
  scan state will be SCAN_DONE. So when the constructor of ElementEnum
  returns, the scan state (if no error occurred) will be one of

  If we have holdability: The state SCAN_HOLD_INIT can only happen if
  a commit happens *before* an initial next() is performed, so that
  state can not happen for this scan. Also, I believe there are no
  state transitions possible back to SCAN_INIT or SCAN_HOLD_INIT once
  a next() is performed and {SCAN_INPROGRESS, SCAN_DONE} is reached (I
  did some inspection).

* Now, when ElementEnum#nextElement() is attempted, there are four cases
     n1) holdability and no commit has happened 
     n2) holdability and a commit happened
     n3) no holdability and  no commit has happened 
     n4) no holdability and a commit happened
  n1) Scan state should be either 
      SCAN_INPROGRESS (and hasMore == true) or 
      SCAN_DONE (and hasMore == false).

      If hasMore == false, NoSuchElementException is thrown.  
      If hasMore == true, state should be SCAN_INPROGRESS,
      isPositioned() returns true and the fetch will succeed. Also a
      new next() is performed which moves the state to one of

  n2) Scan state should be either 
      SCAN_HOLD_INPROGRESS┬╣ (and hasMore == true) or 
      SCAN_DONE (and hasMore == false).

      If hasMore == false, NoSuchElementException is thrown. This
      would not normally happen, since nextElement would not be
      called, cf, hasMoreElements().

      If hasMore == true, state should be SCAN_HOLD_INPROGRESS: Our
      predicate "(keepAfterCommit && !scan.isPositioned()" == true and
      we reopen scan before doing the fetch, which should succeed.  A
      new next() is performed which moves the state to one of

      ┬╣see GenericScanController#closeForEndTransaction().

  n3) Similar to n1, except isPositioned() is not called.

  n4) Scan state should be SCAN_DONE,
      cf. GenericScanController#closeForEndTransaction.
      In this case, nextElement is never called; the fact that the
      result set is closed is caught at a higher level, e.g. in
      BasicNoPutResultSetImpl#getNextRow() for the DISTINCT case
      (SQLState.LANG_RESULT_SET_NOT_OPEN, ca line 463).

      Even if it were reached, no attempt to reopen it would be
      performed (not held), and scan.fetch would throw
      SQLState.AM_SCAN_NOT_POSITIONED, which is OK.

* There is a public method setScanState(state) in
  GenericScanController which could conceivably be used to effect
  other state transitions, but I checked, and it is only used by
  HeapScan#positionAtRowLocation to transition from
  SCAN_HOLD_INPROGRESS back to SCAN_INPROGRESS (and for a similar
  purpose by HeapCompressScan).

* Now, for your question, would it better (safer) to explicitly test
  for SCAN_HOLD_INPROGRESS with a method called, say,
  heldAcrossCommit? I believe my analysis above shows that when the
  call to isPositioned is performed, there are only two possible
  states the scan can be in: SCAN_HOLD_INPROGRESS or SCAN_INPROGRESS,
  so they are equally safe (as the code stands right now, anyway).

  Now, whether there is, or were to be, some *other* way of closing
  the scan (moving it to SCAN_DONE without also closing the result
  set), I don't know, but if it were, positionAtRowLocation will
  actually try to reopen the scan (not sure if this is good or not ;),
  however scan.fetch() would fail with
  SQLState.AM_SCAN_NOT_POSITIONED. If we use your approach, we would
  not attempt to reopen in such a case, which may be safer.

  I agree heldAcrossCommit has the benefit of avoiding the test for
  holdability as you indicate, which does make for easier reading of
  the logic. I think I went for is isPositioned from a vague feeling
  this was potentially more generally useful..

  Maybe a (even more) generally useful method could be:
    public boolean isHeldAfterCommit() throws StandardException
        return (scan_state == SCAN_HOLD_INIT ||
                scan_state == SCAN_HOLD_INPROGRESS);

  that is, if this returns true, the scan can always be reopened
  (although in the case at hand, only the second state tested for may
  occur as shown above).

  What do you think?

> 2) .... I was wondering, though, if it would be worth it to check
> that the correct *error* is thrown if a call to "next()" is made
> after a commit when holdOverCommit is *false*. Or is that already
> tested somewhere else?

As I mentioned above, the error is caught higher up in language layer
(open result set or not), so I belive this is (or should be ;-) tested
for elsewhere. I manually modified the test to verify that for all
three variants (join, distinct and cursor), this happened. If you this
it is still advisable, I can add those tests cases to SpillHash.

If you agree, I will make a version of the patch with the new
isHeldAfterCommit outlined above.

> org.apache.derby.impl.store.access.BackingStoreHashTableFromScan does not honor ResultSet
> -----------------------------------------------------------------------------------------------------
>                 Key: DERBY-2462
>                 URL: https://issues.apache.org/jira/browse/DERBY-2462
>             Project: Derby
>          Issue Type: Bug
>          Components: Store
>    Affects Versions:,,,,
>         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.

View raw message