db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andreas Korneliussen <Andreas.Kornelius...@Sun.COM>
Subject Re: [jira] Updated: (DERBY-1067) support holdable Scrollable Updatable Resultsets
Date Mon, 06 Mar 2006 17:42:25 GMT
Mike Matrigali (JIRA) wrote:
>      [ http://issues.apache.org/jira/browse/DERBY-1067?page=all ]
> 
> Mike Matrigali updated DERBY-1067:
> ----------------------------------
> 

Hi,
Thanks for the review comments. See some inline comments.


>     Description:   (was: I am reviewing this patch.  (mike matrigali)
> 2 major concerns with this patch:
> 1) The timestamp is implemented in runtime, but not persistent.  The container
>    can be thrown away as soon as someone is not referencing it, which I believe
>    can happen in the holdable cursor case.   If you want to implement the
>    timestamp then I think you have to add to the container header
>    (see FileContainer line 278 for container header description), and
>    follow code that updates estimated row count for how it is updated and
>    read.  Note that doing this is an UPGRADE issue, and you should think
>    about soft vs. hard upgrade for this feature.
> 

I am addressing this issue, by writing the time stamp in the header as 
you suggested.

>    For more comments about upgrade I need to know your plan.  On soft upgrade
>    will timestamp be bumped or not.  I would prefer that it not be changed.

Right now, it would be bumped whenever someone executes online compress.
Can you run online compress during soft upgrade ? I guess the 
alternative would be to prevent hodable SUR during soft upgrade.


>    The current assumption for "unused" fields in store is that they are
>    guaranteed with a specific value (usually 0) before an upgrade.  So
>    on hard upgrade we know the starting value.  Also if you change it in
>    soft upgrade then you have to make sure that all previous of 10.1 don't
>    have a problem with that field not being 0 - sometimes there are assertions
>    about the field being 0, don't know for sure in this particular case.
> 

Yes, I would need to check that. I did not find any assertions on this 
field in the current code, however I have not yet checked with all 
versions of 10.1.

> 2) I would have expected tests specific to this change associated with the
>    patch.
>

Yes, currently I have provided some black box tests for holdable 
resultsets in HoldabilityTest.  They will check this feature by running 
online compress on a table where we have a holdable SUR. This test of 
course requires the rest of the SUR implementation to actually test this.

Did you also expect some unit tests for store ?

>    some testing areas of concern:
>    o soft upgrade, make sure 10.1 works correctly on a 10.2 soft upgrade run.

I guess this could be done by extending the phaseTester, by doing a 
online compress in phase 2 (soft upgrade). In phase 3, the old 10.1 
version would be started, and we should then see that it handles that 
the value in the FileContainer header has been changed.

>    o what happens on timestamp overflow?
> 

The next timestamp will be Long.MIN_VALUE, the next timestamp after that 
will be Long.MIN_VALUE +1. I think you would need to run very many 
compress operations on the table to actually test overflow, unless I 
inject some state.

> 
> minor comments:
> 
> general comments:
> I would have rather seen the timestamp tied to the reusable rowlocation
> concept rather than tied to compress.  While true the only thing in the
> current code that breaks this is compress, so this may just be my itch.
> 

Maybe I could do that, right now I have not. Is RowLocation known to the 
  Container ? The compress concept seems to be.

> should timestamp be more "time" related.  A single db may reuse a containerid,
> but only after a shutdown/reboot cycle.  A time based timestamp would mean
> the new container timestamp would be different from the old one.  Probably
> does not matter for held cursors, but what makes sense for the generic new
> timestamp feature?
> 

Instead of using a long, do you think it would be good to introduce a 
new interface similar to PageTimeStamp (instead: ContainerTimeStamp) ?

> questions:
> why do you get the timestamp for the open cursor at close rather than open?
> 
I will change this and initialize it when the cursor opens.

> 
> style comments:
> don't want to start coding style arg here, and admit not all store code is
> perfect.  Most the access code is consistent though, and uses the brace on
> separate line standard.
> 

No problem, I will update the style for my changes to put braces on a 
separate line.

--Andreas

Mime
View raw message