[
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
{SCAN_INIT,
SCAN_INPROGRESS,
SCAN_DONE,
SCAN_HOLD_INIT,
SCAN_HOLD_INPROGRESS}
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
{SCAN_INPROGRESS, SCAN_DONE}.
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
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
{SCAN_INPROGRESS, SCAN_DONE}.
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
{SCAN_INPROGRESS, SCAN_DONE}.
¹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 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.
