db-derby-dev mailing list archives

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

Mike Matrigali updated DERBY-1067:
----------------------------------

    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.

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

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

   some testing areas of concern:
   o soft upgrade, make sure 10.1 works correctly on a 10.2 soft upgrade run.
   o what happens on timestamp overflow?


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.

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?

questions:
why do you get the timestamp for the open cursor at close rather than open?


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.

GenericScanController.java, reOpenAfterEndTransaction()  - coding style does
     not match surrounding code (ie. brackets on same lines and if condition
     on same line).
GenericScanController.java, closeForEndTransaction()  - coding style does
     not match code in same routine.  I think minimum style in same routine
     should be consistent.)

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. 

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

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

   some testing areas of concern: 
   o soft upgrade, make sure 10.1 works correctly on a 10.2 soft upgrade run. 
   o what happens on timestamp overflow? 


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. 

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? 

questions: 
why do you get the timestamp for the open cursor at close rather than open? 


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. 

GenericScanController.java, reOpenAfterEndTransaction() - coding style does 
     not match surrounding code (ie. brackets on same lines and if condition 
     on same line). 
GenericScanController.java, closeForEndTransaction() - coding style does 
     not match code in same routine. I think minimum style in same routine  
 


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


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