db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andreas Korneliussen (JIRA)" <derby-...@db.apache.org>
Subject [jira] Updated: (DERBY-1067) support holdable Scrollable Updatable Resultsets
Date Wed, 15 Mar 2006 12:50:41 GMT
     [ http://issues.apache.org/jira/browse/DERBY-1067?page=all ]

Andreas Korneliussen updated DERBY-1067:
----------------------------------------

    Attachment: DERBY-1067v3.diff

Thanks for the review.

Attached is a new patch which improves the unit test as requested. In addition, the unit test
now also uses fetch() to verify the contents of the row. In addition, the patch fixed one
comment (due to question/comment 4).

Below are answers to your questions (inline):

> 
> Øystein Grøvlen commented on DERBY-1067:
> ----------------------------------------
> 
> Patch looks good, but I have a few questions/comments:
> 
> 1. Is the idea that as long as you go forward and do not reposition,
>    one should be allowed to proceed after a compress?  I ask because I
>    have not quite understood why invalid row locations only seem to
>    have effect on positionOnRowLocation() and not next().
> 

Yes. This is the idea. next() positions the scan to the next valid row. If I did anything
to invalidate next(), it would have side-effects on forward only cursors.

> 2. It seems to me that positionOnRowLocation() can return false in two
>    cases: 
>       1) The row location is no longer valid
>       2) The record at this location has been deleted
>    Does not a caller need to distinguish between these two cases?
>

No. In order for a RowLocation to be invalidated,the row has to be deleted, either as part
of a compress (delete+insert) or as part of delete+purge.
The caller does not need to distinguish between these two cases.
 
> 3. I suggest to use something even more generic than
>    reusableRecordIdSequenceNumber.  How about recordIdVersion or
>    something like that?  I agree that what we worry about here is
>    reuse of record ids, but I think this mechanism could be used for
>    other purposes too.
>

I think I will leave it as is, since I have previously been asked to link this with reusable
record ids.  However, I do not mind that anyone uses this mechanism for something else, and
as part of that issue renames it to something else, like recordIdVersion. 
 
> 4. reopenAfterEndTransaction has a comment that says "Only reopen
>    holdable conglomerates".  I guess it is not the conglomerate that
>    is holdable, but the way it was opened.
> 
Yes. I fixed the comment.

> 5. The following is not an objection to this patch, but the existing
>    code: HeapCompressScan.fetchRowsForCompress() has copied a lot of
>    code from GenericScanController.fetchRows().  This requires this
>    patch to update both methods.  Is it not possible to organize this
>    in a way that avoids this code duplication?  Mike, can comment on
>    this?
> 
> 6. FileContainer header:  
>       a) When looking at the list of fields, it seems like there will
>          be only 10 bytes of spare space left.

One long field (8 bytes) + one integer (4 bytes), should be 12.
Do not see how it would only be 10 bytes left.

>       b) We will now have a spare1 and a spare3 field, but no spare2
>          field.  I guess that may confuse some people, but renaming
>          spare3 to spare2 may also create confusion.  I am not sure
>          what is best.
> 
> 7. Unit test:
>       a) I think it would also be good with a test that does next()
>          after a compress and verifies that it is positioned at the
>          correct row.  (Or maybe this is already part of the SUR
>          testsuite?)
Added.
>       b) Comments says that an index is created on the conglomerate.
>          I do not see any code for that.
Fixed comment.
>       c) Why does one fetch the row locations when they are not used
>          for anything?  Why not use plain insert() instead of
>          insertAndFetchLocation()?
> 
Because this was cut'npasted from heldCursor. I have changed it to use insert() instead.

> 8. phaseTester:  Any particular reason a prepared statement is used
>    for the compresssion?
> 
No, it can be used with plain statements also.

> support holdable Scrollable Updatable Resultsets
> ------------------------------------------------
>
>          Key: DERBY-1067
>          URL: http://issues.apache.org/jira/browse/DERBY-1067
>      Project: Derby
>         Type: Sub-task
>     Reporter: Andreas Korneliussen
>     Assignee: Andreas Korneliussen
>  Attachments: DERBY-1067.diff, DERBY-1067.stat, DERBY-1067v2.diff, DERBY-1067v2.stat,
DERBY-1067v3.diff, derbyall_report.txt
>


-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators:
   http://issues.apache.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see:
   http://www.atlassian.com/software/jira


Mime
View raw message