MIME-Version: 1.0
Sender: amdragon@gmail.com
In-Reply-To: <1315445674.32058.183.camel@delen>
References: <1315445674.32058.183.camel@delen>
Date: Thu, 8 Sep 2011 22:42:32 -0400
Message-ID:
 <CAH-f9WtuYYuksfTHL1ZMmoRF6kyL7iFu9BXDzGn-wWGniSvf9w@mail.gmail.com>
Subject: Re: Patch: Flush and Reopen
From: Austin Clements <amdragon@mit.edu>
To: Martin Owens <doctormo@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Cc: Paul Tagliamonte <paultag@ubuntu.com>,
	Notmuch developer list <notmuch@notmuchmail.org>
Precedence: list

On Wed, Sep 7, 2011 at 9:34 PM, Martin Owens <doctormo@gmail.com> wrote:
> Hello,
>
> Per discussions on irc, I've attached a patch for your consideration
> which allows the developer to run two new database commands;
>
> notmuch_database_flush - used on read_write databases to commit changes
> to disk
> notmuch_database_reopen - used on read_only databases to reload from the
> disk
>
> Used in conjunction they can allow access by multiple programs at the
> same time. There are also changes to the python bindings to add these
> two methods to the database class.
>
> Regards, Martin Owens

Ages ago, there was a lot of discussion about improving notmuch's
concurrency so that other commands could run simultaneously with
long-running operations like notmuch new (whereas currently many
commands always abort and others are likely to fail with a concurrent
modification exception).  This patch, combined with the atomicity
changes (assuming they ever get in), would make it possible to
implement the concurrency protocol I suggested way back in
id:"AANLkTi=KOx8aTJipkiArFVjEHE6zt_JypoASMiiAWBZ6@mail.gmail.com".

A few comments on your patch:

--- a/debian/control
+++ b/debian/control
@@ -5,7 +5,7 @@ Maintainer: Carl Worth <cworth@debian.org>
 Uploaders: Jameson Graef Rollins <jrollins@finestructure.net>, martin
f. krafft <madduck@debian.org>,
 	   David Bremner <bremner@debian.org>
 Build-Depends: debhelper (>= 7.0.50~), pkg-config, libxapian-dev,
- libgmime-2.4-dev, libtalloc-dev, libz-dev,  python-all (>= 2.6.6-3~),
+ libgmime-2.4-dev, libtalloc-dev, libz-dev,  python-all (>= 2.6.6),

Did you mean to change this?

--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,37 @@ notmuch_database_open (const char *path,
 }

 void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
 {
     try {
-	if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
-	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
+        if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_ONLY)
+            (static_cast <Xapian::WritableDatabase *>
(notmuch->xapian_db))->reopen();

This cast will fail.  Shouldn't this just be a wrapper around
notmuch->xapian_db->reopen?

--- a/lib/query.cc
+++ b/lib/query.cc
@@ -185,10 +185,14 @@ notmuch_query_search_messages (notmuch_query_t *query)
 	messages->iterator_end = mset.end ();

 	return &messages->base;
-
+    } catch (const Xapian::DatabaseModifiedError &error) {
+      fprintf (stderr, "Database changed, you need to reopen it
before performing queries.\n");
+      notmuch->exception_reported = TRUE;
+      talloc_free (messages);
+      return NULL;

There are many places where DatabaseModifiedError could be thrown
besides this function.  This needs to be handled more systematically.

Also, your patch has a number of code- and patch-formatting problems.
Please take a look at the (work-in-progress) patch formatting
guidelines at http://notmuchmail.org/patchformatting/ .  For the code
style, I'd recommend looking at the code around what you're changing.
For example, notmuch uses tab indentation and a space before the open
paren of an argument list.