From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 3221E431FAF for ; Thu, 7 Aug 2014 14:55:58 -0700 (PDT) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: 0 X-Spam-Level: X-Spam-Status: No, score=0 tagged_above=-999 required=5 tests=[none] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UlEBHfOD1Xny for ; Thu, 7 Aug 2014 14:55:50 -0700 (PDT) Received: from guru.guru-group.fi (guru.guru-group.fi [46.183.73.34]) by olra.theworths.org (Postfix) with ESMTP id 9A728431FAE for ; Thu, 7 Aug 2014 14:55:49 -0700 (PDT) Received: from guru.guru-group.fi (localhost [IPv6:::1]) by guru.guru-group.fi (Postfix) with ESMTP id 8D160100051; Fri, 8 Aug 2014 00:55:37 +0300 (EEST) From: Tomi Ollila To: Austin Clements , notmuch@notmuchmail.org Subject: Re: [PATCH v3 00/13] Implement and use database "features" In-Reply-To: <1406859003-11561-1-git-send-email-amdragon@mit.edu> References: <1406859003-11561-1-git-send-email-amdragon@mit.edu> User-Agent: Notmuch/0.18.1+25~gdaf4b6f (http://notmuchmail.org) Emacs/24.3.1 (x86_64-unknown-linux-gnu) X-Face: HhBM'cA~ MIME-Version: 1.0 Content-Type: text/plain X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Aug 2014 21:55:58 -0000 On Fri, Aug 01 2014, Austin Clements 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)) 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)); > > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > http://notmuchmail.org/mailman/listinfo/notmuch