Date: Fri, 8 Aug 2014 14:18:57 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Tomi Ollila <tomi.ollila@iki.fi>
Subject: Re: [PATCH v3 00/13] Implement and use database "features"
Message-ID: <20140808181856.GA17169@mit.edu>
References: <1406859003-11561-1-git-send-email-amdragon@mit.edu>
	<m2a97gt15y.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <m2a97gt15y.fsf@guru.guru-group.fi>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Tomi Ollila on Aug 08 at 12:55 am:
> On Fri, Aug 01 2014, Austin Clements <amdragon@MIT.EDU> wrote:
> 
> > This is v3 of id:1406652492-27803-1-git-send-email-amdragon@mit.edu.
> > This fixes one issue and tidies up another thing in
> > notmuch_database_upgrade I found while working on change tracking
> > support.  Most of the patches are logically identical to v2, but the
> > changes ripple through the patch context, so I'm sending a new series.
> >
> > First, this updates notmuch->features before starting the upgrade,
> > rather than after, so that functions called by upgrade will use the
> > new database features instead of the old (this didn't matter in this
> > series because nothing modified the database differently depending on
> > features).  Second, this combines multiple _notmuch_message_sync calls
> > into one, which cleans up the code, should further improve upgrade
> > performance, and makes way for additional per-message upgrades.
> 
> This series looks good to me. I looked through the diffs a few times and
> notmuch_database_upgrade() in lib/database.cc to see that in full.
> Tests pass (also T530-upgrade now that I downloaded that one test database.)
> 
> I googled answers to few questions along the review; one thing still
> interests me -- is there potential to have speed/memory problems
> when doing upgrade transaction in large/very large databases. And
> how long will the (final) commit_transaction() take (i.e how
> many times handle_sigalrm() is called while that is in progress...)
> 
> (my guess is that this transaction just builds a new revision and
> if it is never committed the revision is never used -- and data is
> written there in some batches of suitable size -- so memory usage
> and transaction commit time is O(1))

Your guess is basically right.  Xapian periodically flushes stuff to
disk during a transaction basically just like it does when not in a
transaction; AFAIK the only difference is when it flushes out the new
revision number and updated free block metadata.  Hence, speed and
memory use aren't affected by the use of a transaction, and
commit_transaction isn't particularly sensitive to how big the
transaction is.

> Tomi
> 
> >
> > The diff from v2 is below (excluding patch 2, which David pushed).
> >
> > diff --git a/lib/database.cc b/lib/database.cc
> > index b323691..d90a924 100644
> > --- a/lib/database.cc
> > +++ b/lib/database.cc
> > @@ -1252,6 +1252,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >      /* Perform the upgrade in a transaction. */
> >      db->begin_transaction (true);
> >  
> > +    /* Set the target features so we write out changes in the desired
> > +     * format. */
> > +    notmuch->features = target_features;
> > +
> >      /* Perform per-message upgrades. */
> >      if (new_features &
> >  	(NOTMUCH_FEATURE_FILE_TERMS | NOTMUCH_FEATURE_BOOL_FOLDER)) {
> > @@ -1280,7 +1284,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  		if (filename && *filename != '\0') {
> >  		    _notmuch_message_add_filename (message, filename);
> >  		    _notmuch_message_clear_data (message);
> > -		    _notmuch_message_sync (message);
> >  		}
> >  		talloc_free (filename);
> >  	    }
> > @@ -1289,10 +1292,10 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	     * probabilistic and stemmed. Change it to the current
> >  	     * boolean prefix. Add "path:" prefixes while at it.
> >  	     */
> > -	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER) {
> > +	    if (new_features & NOTMUCH_FEATURE_BOOL_FOLDER)
> >  		_notmuch_message_upgrade_folder (message);
> > -		_notmuch_message_sync (message);
> > -	    }
> > +
> > +	    _notmuch_message_sync (message);
> >  
> >  	    notmuch_message_destroy (message);
> >  
> > @@ -1348,7 +1351,6 @@ notmuch_database_upgrade (notmuch_database_t *notmuch,
> >  	}
> >      }
> >  
> > -    notmuch->features = target_features;
> >      db->set_metadata ("features", _print_features (local, notmuch->features));
> >      db->set_metadata ("version", STRINGIFY (NOTMUCH_DATABASE_VERSION));
> >