some quick comments while I have time, maybe some more later. Sorry
I missed this note yesterday.
Andreas Korneliussen wrote:
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.
I am not really worried about "during" soft upgrade. Basically I mean
that a customer has decided to not upgrade the db so version of the
db will be at 10.1, I refer to this has
soft upgrade. In that mode one can run 10.2 server against the db but
no changes to the state of the database are made such that 10.1 can not
be subsequently run. Changing the header is obviously a database
upgrade to 10.2. The safest is just to not allow the change unless the
db is upgraded to 10.2. The way I usually think about it is if the
change to the database is not something we think should be put in a
bug fix point release then it is not something we should put in
a soft upgrade. In this case that would mean only updating the
timestamp if the version of the db is 10.2.
So first I need to understand if your timestamp gets set if version of
db is 10.1. And will scrollable updatable result set code be executed
in a version 10.1 db?
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 test is necessary, i am not sure if we need 2 sets. Of course the
interesting tests are when row locations are invalidated but that comes
with your other jira item.
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.
i agree, very rare especially using a long.
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.
RowLocations are not known by container, but containers support
non-reusable record id's which is what row locations are built on.
These are a page level container specific implementation detail.
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
