db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Oystein.Grov...@Sun.COM (Øystein Grøvlen)
Subject Re: [jira] Commented: (DERBY-239) Need a online backup feature that does not block update operations when online backup is in progress.
Date Mon, 31 Oct 2005 18:48:58 GMT
>>>>> "ST" == Suresh Thalamati <suresh.thalamati@gmail.com> writes:

    ST> Hi Øystein,
    ST> Thanks for reviewing the patch. My answers are in-line...

    ST> Øystein Grøvlen (JIRA) wrote:

...

    >> * Intuitively, it seems wrong to hard-code "seg0", but I see that
    >> this is done all over the code.


    ST> Could you  please file a  JIRA entry for  this one. I noticed  it too,
    ST> hard coding  is definitely  not the  correct way here.  One of  us can
    ST> clean up this.

Done. Derby-664.

...

    >> * backup(File ...) seems like it would create an endless recursion
    >> if called.  Fortunately, it seems like it never will be
    >> called. Why do we need the methods with a File parameter instead
    >> of a String.  The system procedures uses the String variant.
    >> Maybe we could just remove the File variant?

    ST> I think  they are  left overs from  the old cloudscape  versions, File
    ST> variant is  not supported any  more , these  can be cleaned  up. There
    ST> should  be some File  variant calls  in access  layer also,  Could you
    ST> please  file Jira  for this  cleanup,  so that  these won't  forgotten
    ST> again.

Done. Derby-665.

...

    >> BaseDataFileFactory.java:

    >> * I do think basing which files to back up on the contents of the
    >> seg0 directory is very robust.  What if someone by accident has
    >> written a file with a name that matches the pattern you are
    >> looking for.  Then I would think you may get a very strange
    >> error message that may not be easy to resolve.  Could not this
    >> be based on some system catalog?


    ST> I think  users should  not be creating/deleting  any files in  SEG0 to
    ST> start with. Pattern matching is just a simple precaution. I thought of
    ST> scanning the catalogs  to find the container to  backup, but could not
    ST> convince my self that it is required at least for now.

I agree that users should not mess with seg0.  However, I think part
of being an easy-to-use database, is to be able to give meaningful
error messages when users mess things up.

    ST> If you  think scanning the  catalogs is more robust,  this enhancement
    ST> can be done at later point of time easily.

Ok. I will see if I feel the itch.  8-)

    >> Another scenario is if someone
    >> by accident deletes a file for a table that is not accessed very
    >> often.  A later backup will then not detect that this file is
    >> missing.  Since the backup is believed to be succesful, the
    >> latest backup of this file may be deleted.
    >> 

    ST> Backup is as good as the database in this case :-) If users access the
    ST> deleted file table from the backup DB , it will fail with NO container
    ST> error, as it will on the main database.


    ST> By using the catalog approach also backup can only throw a warning
    ST> about the  deleted container. Once the user deletes a container file ,
    ST> there is no way out, users can not even drop that table. I don't think
    ST> making  backup  fail  forever   when  this  scenario  occurs  will  be
    ST> acceptable to the users.

I agree, but detecting in on backup, gives the user the option of
recovering the table by restoring the previous bakcup.

    >> FileContainer.java:
    >> * I cannot find any backup-specific about getPageForBackup() so I
    >> think a more general name would be better (e.g, getLatchedPage).
    >> 


    ST> Yes, currently this routine does not have any backup specific stuff in
    ST> this  routine. I  would  like to  get  the page  from  the cache  with
    ST> specific weight for  the backup page in future,  when the weight based
    ST> cache mechanism is implemented.

In that case, I would suggest that the weight be a parameter to the
method, not part of the name.  

    >> RAFContainer.java:
    >> * The changes to this file seems not to be quite in line with some
    >> of the original design philosophies.  I am not sure that is
    >> necessarily bad, but it would be nice to here the arguments for
    >> doing it this way.  More specifically:
    >> - While RAFContainer so far has used the
    >> StorageRandomAccessFile/StorageFile abstractions, backup
    >> use RandomAccessFile/File directly.  Is there a particular
    >> reason for that?

    ST> Yes. Currently there is No API  available for the users to specify the
    ST> type  of Storage  Factory  to use  for  the backup.  It  will be  good
    ST> enhancement to add later.


    ST> Online backup implementation is doing what the current backup does;
    ST> i.e use the  disk IO calls directly , instead  of the database Storage
    ST> Factory. I think one  reason to do this way is if  a database is using
    ST> in-memory  Storage Factory,  users can  backup  the database  on to  a
    ST> disk.  If Storage Factory  used by  the Database  is java.io.*  , then
    ST> backup is  doing the  same without going  through the  Storage Factory
    ST> interface.

Makes sense.  If one want a StorageFactory for backup, one can add that
later.

    >> - In order to be able to backup a page, new methods have
    >> been added to FileContainer and BasePage/CachedPage, and
    >> the RAFContainer is doing latching of pages.  This
    >> increases coupling to other modules, have alternative
    >> designs been considered?


    ST> I was considering doing following minor changes here:

    ST> 1) Instead of getting pageData byte array from the Page Object, make
    ST> a request to the page to write it self to backup(like page.backup(....).

Sounds good to me.  Then much the same structure is used for backup as
for clean.

    ST> 2)  Move  the backing  up  and  unlatching of  the  page  code to  the
    ST> FileContainer.java.


    ST> I have  not thought of  any other better  ways to do design  this, any
    ST> ideas will be helpful.

Could the backup code be in a separate class instead of part of
RAFContainer?  It does not seem to use much of RAFContainer's internal
state. 

...

-- 
Øystein


Mime
View raw message