Patch: Flush and Reopen

Patch: Flush and Reopen

[Martin Owens]

[-- Attachment #1: Type: text/plain, Size: 496 bytes --]

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

[-- Attachment #2: 0001-Add-flush-and-reopen-methods-to-the-libnotmuch-and-t.patch --]
[-- Type: text/x-patch, Size: 6897 bytes --]

From 92b89aad3101faa42c6fdca9b6683ebfa42ab3b0 Mon Sep 17 00:00:00 2001
From: Martin Owens <doctormo@gmail.com>
Date: Wed, 7 Sep 2011 21:15:54 -0400
Subject: [PATCH] Add flush and reopen methods to the libnotmuch and to the python bindings. Flush allows read_write databases to commit their changes, forcing a flush to disk and reopen allows a read_only database to reread from the disk.

Also added a handler for Xapian DatabaseChangedError which occurs when the flush happens to read only databases.
---
 bindings/python/notmuch/database.py |   10 ++++++++++
 debian/changelog                    |    6 +++++-
 debian/control                      |    2 +-
 debian/libnotmuch1.symbols          |    2 ++
 lib/database.cc                     |   33 ++++++++++++++++++++++++++-------
 lib/notmuch.h                       |    8 ++++++++
 lib/query.cc                        |   10 +++++++---
 7 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f18ca14..b6c1c31 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -432,6 +432,16 @@ class Database(object):
 
         return Query(self, querystring)
 
+    def reopen(self):
+        """Reopens a read only database, when the data has changed, this is required."""
+        if self._db is not None:
+            nmlib.notmuch_database_reopen(self._db)
+
+    def flush(self):
+        """Flushes the search database, only available when in read-write."""
+        if self._db is not None:
+            nmlib.notmuch_database_flush(self._db)
+
     def __repr__(self):
         return "'Notmuch DB " + self.get_path() + "'"
 
diff --git a/debian/changelog b/debian/changelog
index c2b7552..565edfb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,8 +1,12 @@
 notmuch (0.8~rc1-1) experimental; urgency=low
 
+  [ David Bremner ]
   * Upstream release candidate.
 
- -- David Bremner <bremner@debian.org>  Tue, 06 Sep 2011 22:24:24 -0300
+  [ Martin Owens (DoctorMO) ]
+  * Re-new-world-order
+
+ -- Martin Owens (DoctorMO) <doctormo@gmail.com>  Wed, 07 Sep 2011 18:19:50 -0400
 
 notmuch (0.7-1) unstable; urgency=low
 
diff --git a/debian/control b/debian/control
index 03afdf4..a72fe1b 100644
--- 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),
  emacs23-nox | emacs23 (>=23~) | emacs23-lucid (>=23~)
 Standards-Version: 3.9.2
 Homepage: http://notmuchmail.org/
diff --git a/debian/libnotmuch1.symbols b/debian/libnotmuch1.symbols
index 05d86e6..7e42b0e 100644
--- a/debian/libnotmuch1.symbols
+++ b/debian/libnotmuch1.symbols
@@ -3,6 +3,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
  notmuch_database_close@Base 0.3
  notmuch_database_create@Base 0.3
  notmuch_database_find_message@Base 0.3
+ notmuch_database_flush@Base 0.3
  notmuch_database_get_all_tags@Base 0.3
  notmuch_database_get_directory@Base 0.3
  notmuch_database_get_path@Base 0.3
@@ -10,6 +11,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
  notmuch_database_needs_upgrade@Base 0.3
  notmuch_database_open@Base 0.3
  notmuch_database_remove_message@Base 0.3
+ notmuch_database_reopen@Base 0.3
  notmuch_database_upgrade@Base 0.3
  notmuch_directory_destroy@Base 0.3
  notmuch_directory_get_child_directories@Base 0.3
diff --git a/lib/database.cc b/lib/database.cc
index 9c2f4ec..58b6c75 100644
--- 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();
     } catch (const Xapian::Error &error) {
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
-	}
+        if (! notmuch->exception_reported) {
+            fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
+                error.get_msg().c_str());
+        }
     }
+}
 
+void
+notmuch_database_flush (notmuch_database_t *notmuch)
+{
+    try {
+        if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
+            (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush();
+    } catch (const Xapian::Error &error) {
+        if (! notmuch->exception_reported) {
+            fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
+                error.get_msg().c_str());
+        }
+    }
+}
+
+void
+notmuch_database_close (notmuch_database_t *notmuch)
+{
+    notmuch_database_flush (notmuch);
     delete notmuch->term_gen;
     delete notmuch->query_parser;
     delete notmuch->xapian_db;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..ca87b89 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -171,6 +171,14 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
+/* Reopen a read_only database when the data source has changed */
+void
+notmuch_database_reopen (notmuch_database_t *database);
+
+/* Force a flush of the xapian database, also done on close */
+void
+notmuch_database_flush (notmuch_database_t *database);
+
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
 void
diff --git a/lib/query.cc b/lib/query.cc
index 6f02b04..d116367 100644
--- 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;
     } catch (const Xapian::Error &error) {
-	fprintf (stderr, "A Xapian exception occurred performing query: %s\n",
-		 error.get_msg().c_str());
+	fprintf (stderr, "A Xapian exception occurred performing query: %s '%s'\n",
+		 error.get_msg().c_str(), error.get_type());
 	fprintf (stderr, "Query string was: %s\n", query->query_string);
 	notmuch->exception_reported = TRUE;
 	talloc_free (messages);
-- 
1.7.1

Re: Patch: Flush and Reopen

[Austin Clements]

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.

Re: Patch: Flush and Reopen

[Martin Owens]

Hey Austin,

Thanks for the review :-) I'll attempt to fix some of the style
guidelines, but see below.

On Thu, 2011-09-08 at 22:42 -0400, Austin Clements wrote:
> Did you mean to change this?

This fails to build on Ubuntu maverick with the extra .3 and I see no
reason to have that sub-minor version. Pushing it in would probably be
useful unless there is a real reason.

> This cast will fail.  Shouldn't this just be a wrapper around
> notmuch->xapian_db->reopen? 

I'm a python programmer, c wrappers are beyond me unfortunately. Your
help implementing that would be really good.

Best regards, Martin Owens

Re: Patch: Flush and Reopen

[David Bremner]

Thanks for sending your patch. We are kind of limping along as far as
patch integration at the moment. It would help if you could seperate out
the parts you think are bugs from the new features part of your patches, 
then the bug-fixes could get looked at sooner.

On Thu, 08 Sep 2011 22:54:21 -0400, Martin Owens <doctormo@gmail.com> wrote:

> This fails to build on Ubuntu maverick with the extra .3 and I see no
> reason to have that sub-minor version. Pushing it in would probably be
> useful unless there is a real reason.

I'm afraid in this case your change conflicts with the documentation
for dh_python2, so I'd some more convincing before I pushed that.

I was also puzzled by your changes to debian/changelog. We really need
justification for every little change introduced by the patch; this is
another reason it helps to split things up a bit.

d

Re: Patch: Flush and Reopen

[Martin Owens]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

On Fri, 2011-09-09 at 08:06 -0300, David Bremner wrote:
> I was also puzzled by your changes to debian/changelog. We really need
> justification for every little change introduced by the patch; this is
> another reason it helps to split things up a bit. 

That probably was unintended. See attached for the all new slimmed down
patch.

I've removed the debian control change, the changelog change, the extra
error catch in query.cc and put that cast right.

I'll leave the version of python-all for someone else.

Best Regards, Martin Owens

[-- Attachment #2: 0001-Add-flush-and-reopen-methods-to-the-libnotmuch-and-t.patch --]
[-- Type: text/x-patch, Size: 4675 bytes --]

From 981b91e9405de2daf35d30d9bbeab0dd62b15683 Mon Sep 17 00:00:00 2001
From: Martin Owens <doctormo@gmail.com>
Date: Fri, 9 Sep 2011 13:51:59 -0400
Subject: [PATCH] Add flush and reopen methods to the libnotmuch and to the python bindings. Flush allows read_write databases to commit their changes, forcing a flush to disk and reopen allows a read_only database to reread from the disk.

Also added a handler for Xapian DatabaseChangedError which occurs when the flush happens to read only databases.
---
 bindings/python/notmuch/database.py |   10 ++++++++++
 debian/libnotmuch1.symbols          |    2 ++
 lib/database.cc                     |   33 ++++++++++++++++++++++++++-------
 lib/notmuch.h                       |    8 ++++++++
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f18ca14..b6c1c31 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -432,6 +432,16 @@ class Database(object):
 
         return Query(self, querystring)
 
+    def reopen(self):
+        """Reopens a read only database, when the data has changed, this is required."""
+        if self._db is not None:
+            nmlib.notmuch_database_reopen(self._db)
+
+    def flush(self):
+        """Flushes the search database, only available when in read-write."""
+        if self._db is not None:
+            nmlib.notmuch_database_flush(self._db)
+
     def __repr__(self):
         return "'Notmuch DB " + self.get_path() + "'"
 
diff --git a/debian/libnotmuch1.symbols b/debian/libnotmuch1.symbols
index 05d86e6..7e42b0e 100644
--- a/debian/libnotmuch1.symbols
+++ b/debian/libnotmuch1.symbols
@@ -3,6 +3,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
  notmuch_database_close@Base 0.3
  notmuch_database_create@Base 0.3
  notmuch_database_find_message@Base 0.3
+ notmuch_database_flush@Base 0.3
  notmuch_database_get_all_tags@Base 0.3
  notmuch_database_get_directory@Base 0.3
  notmuch_database_get_path@Base 0.3
@@ -10,6 +11,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
  notmuch_database_needs_upgrade@Base 0.3
  notmuch_database_open@Base 0.3
  notmuch_database_remove_message@Base 0.3
+ notmuch_database_reopen@Base 0.3
  notmuch_database_upgrade@Base 0.3
  notmuch_directory_destroy@Base 0.3
  notmuch_directory_get_child_directories@Base 0.3
diff --git a/lib/database.cc b/lib/database.cc
index 9c2f4ec..b86bf1a 100644
--- 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::Database *> (notmuch->xapian_db))->reopen();
     } catch (const Xapian::Error &error) {
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
-	}
+        if (! notmuch->exception_reported) {
+            fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
+                error.get_msg().c_str());
+        }
     }
+}
 
+void
+notmuch_database_flush (notmuch_database_t *notmuch)
+{
+    try {
+        if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
+            (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush();
+    } catch (const Xapian::Error &error) {
+        if (! notmuch->exception_reported) {
+            fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
+                error.get_msg().c_str());
+        }
+    }
+}
+
+void
+notmuch_database_close (notmuch_database_t *notmuch)
+{
+    notmuch_database_flush (notmuch);
     delete notmuch->term_gen;
     delete notmuch->query_parser;
     delete notmuch->xapian_db;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..ca87b89 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -171,6 +171,14 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
+/* Reopen a read_only database when the data source has changed */
+void
+notmuch_database_reopen (notmuch_database_t *database);
+
+/* Force a flush of the xapian database, also done on close */
+void
+notmuch_database_flush (notmuch_database_t *database);
+
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
 void
-- 
1.7.1

Re: Patch: Flush and Reopen

[Austin Clements]

On Fri, Sep 9, 2011 at 1:55 PM, Martin Owens <doctormo@gmail.com> wrote:
> That probably was unintended. See attached for the all new slimmed down
> patch.


--- 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::Database *> (notmuch->xapian_db))->reopen();

I was thinking this should probably just be

try {
  notmuch->xapian_db->reopen ();
} catch ...

(indented correctly, of course).  Reopen is a method of
Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
flush, which is a member of the subclass WritableDatabase and hence
requires the cast).  And reopening is a sensible thing to do on both
read-only and read/write databases.

Also, in keeping with notmuch code style, you should use tabs for
indentation and put a space before the open paren argument lists.

Re: Patch: Flush and Reopen

[Martin Owens]

[-- Attachment #1: Type: text/plain, Size: 821 bytes --]

On Fri, 2011-09-09 at 19:40 -0400, Austin Clements wrote:
> 
> (indented correctly, of course).  Reopen is a method of
> Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
> flush, which is a member of the subclass WritableDatabase and hence
> requires the cast).  And reopening is a sensible thing to do on both
> read-only and read/write databases.

See attached for the removal of the cast and conditional for reopen.

> Also, in keeping with notmuch code style, you should use tabs for
> indentation and put a space before the open paren argument lists. 

I couldn't detect a consistant style for the tab/spacing and it confused
me greatly. Especially coming from python where spacing is critical.

I've put in tabs, not sure if I've put in enough in the form required.

Best Regards, Martin Owens

[-- Attachment #2: 0001-Add-flush-and-reopen-methods-to-the-libnotmuch-and-t.patch --]
[-- Type: text/x-patch, Size: 5656 bytes --]

From 0d7180fbf271d696e2105420db87aa2d1f0e72aa Mon Sep 17 00:00:00 2001
From: Martin Owens <doctormo@gmail.com>
Date: Fri, 9 Sep 2011 20:40:21 -0400
Subject: [PATCH] Add flush and reopen methods to the libnotmuch and to the python bindings. Flush allows read_write databases to commit their changes, forcing a flush to disk and reopen allows a read_only database to reread from the disk.

Also added a handler for Xapian DatabaseChangedError which occurs when the flush happens to read only databases.
---
 bindings/python/notmuch/database.py |   10 ++++++++++
 debian/changelog                    |    6 +++++-
 debian/control                      |    2 +-
 debian/libnotmuch1.symbols          |    2 ++
 lib/database.cc                     |   30 ++++++++++++++++++++++++------
 lib/notmuch.h                       |    8 ++++++++
 6 files changed, 50 insertions(+), 8 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f18ca14..b6c1c31 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -432,6 +432,16 @@ class Database(object):
 
         return Query(self, querystring)
 
+    def reopen(self):
+        """Reopens a read only database, when the data has changed, this is required."""
+        if self._db is not None:
+            nmlib.notmuch_database_reopen(self._db)
+
+    def flush(self):
+        """Flushes the search database, only available when in read-write."""
+        if self._db is not None:
+            nmlib.notmuch_database_flush(self._db)
+
     def __repr__(self):
         return "'Notmuch DB " + self.get_path() + "'"
 
diff --git a/debian/changelog b/debian/changelog
index c2b7552..565edfb 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,8 +1,12 @@
 notmuch (0.8~rc1-1) experimental; urgency=low
 
+  [ David Bremner ]
   * Upstream release candidate.
 
- -- David Bremner <bremner@debian.org>  Tue, 06 Sep 2011 22:24:24 -0300
+  [ Martin Owens (DoctorMO) ]
+  * Re-new-world-order
+
+ -- Martin Owens (DoctorMO) <doctormo@gmail.com>  Wed, 07 Sep 2011 18:19:50 -0400
 
 notmuch (0.7-1) unstable; urgency=low
 
diff --git a/debian/control b/debian/control
index 03afdf4..a72fe1b 100644
--- 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),
  emacs23-nox | emacs23 (>=23~) | emacs23-lucid (>=23~)
 Standards-Version: 3.9.2
 Homepage: http://notmuchmail.org/
diff --git a/debian/libnotmuch1.symbols b/debian/libnotmuch1.symbols
index 05d86e6..7e42b0e 100644
--- a/debian/libnotmuch1.symbols
+++ b/debian/libnotmuch1.symbols
@@ -3,6 +3,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
  notmuch_database_close@Base 0.3
  notmuch_database_create@Base 0.3
  notmuch_database_find_message@Base 0.3
+ notmuch_database_flush@Base 0.3
  notmuch_database_get_all_tags@Base 0.3
  notmuch_database_get_directory@Base 0.3
  notmuch_database_get_path@Base 0.3
@@ -10,6 +11,7 @@ libnotmuch.so.1 libnotmuch1 #MINVER#
  notmuch_database_needs_upgrade@Base 0.3
  notmuch_database_open@Base 0.3
  notmuch_database_remove_message@Base 0.3
+ notmuch_database_reopen@Base 0.3
  notmuch_database_upgrade@Base 0.3
  notmuch_directory_destroy@Base 0.3
  notmuch_directory_get_child_directories@Base 0.3
diff --git a/lib/database.cc b/lib/database.cc
index 9c2f4ec..557c683 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,36 @@ notmuch_database_open (const char *path,
 }
 
 void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
+{
+	try {
+		notmuch->xapian_db->reopen ();
+	} catch (const Xapian::Error &error) {
+		if (! notmuch->exception_reported) {
+		fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
+		error.get_msg().c_str());
+		}
+	}
+}
+
+void
+notmuch_database_flush (notmuch_database_t *notmuch)
 {
     try {
 	if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
-	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
+		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
     } catch (const Xapian::Error &error) {
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
+		if (! notmuch->exception_reported) {
+		fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
+		error.get_msg().c_str());
+		}
 	}
-    }
+}
 
+void
+notmuch_database_close (notmuch_database_t *notmuch)
+{
+    notmuch_database_flush (notmuch);
     delete notmuch->term_gen;
     delete notmuch->query_parser;
     delete notmuch->xapian_db;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..ca87b89 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -171,6 +171,14 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
+/* Reopen a read_only database when the data source has changed */
+void
+notmuch_database_reopen (notmuch_database_t *database);
+
+/* Force a flush of the xapian database, also done on close */
+void
+notmuch_database_flush (notmuch_database_t *database);
+
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
 void
-- 
1.7.1

Re: Patch: Flush and Reopen

[Austin Clements]

BTW, in the future, you should send patches inline (see the patch
formatting guide I linked to earlier for easy ways to do this).  It
makes them much easier to review and reply to.

Quoth Martin Owens on Sep 09 at  8:43 pm:
> On Fri, 2011-09-09 at 19:40 -0400, Austin Clements wrote:
> > 
> > (indented correctly, of course).  Reopen is a method of
> > Xapian::Database, which is what notmuch->xapian_db is anyway (unlike,
> > flush, which is a member of the subclass WritableDatabase and hence
> > requires the cast).  And reopening is a sensible thing to do on both
> > read-only and read/write databases.
> 
> See attached for the removal of the cast and conditional for reopen.

Great.  That looks like how I would expect a reopen function to work.
I assume you've tested it?

Speaking of testing, I'm not sure what the best way to test this is,
but it needs something.  notmuch's testing system is great for testing
the CLI, but it's poorly suited to lower-level library things.

> > Also, in keeping with notmuch code style, you should use tabs for
> > indentation and put a space before the open paren argument lists. 
> 
> I couldn't detect a consistant style for the tab/spacing and it confused
> me greatly. Especially coming from python where spacing is critical.
> 
> I've put in tabs, not sure if I've put in enough in the form required.

Notmuch is quite consistent about indentation, though the rule may not
be something you'd expect coming from Python.  It uses four space
indentation, but each group of eight spaces is replaced with a tab, so
for example

notmuch_database_reopen (notmuch_database_t *notmuch)
{
____try {
->      notmuch->xapian_db->reopen ();
____} catch (const Xapian::Error &error) {
->      if (! notmuch->exception_reported) {
->      ____fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
->      ->      _____error.get_msg().c_str());
->      }
____}
}

Yeah, it's weird; blame RMS.  If you're using Emacs, the
.dir-locals.el should set this up automatically for you.  (If you're
not using Emacs, the easiest way might be to pop it up in Emacs,
select the code to indent, and do M-x indent-region.)

> diff --git a/debian/changelog b/debian/changelog
> index c2b7552..565edfb 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,8 +1,12 @@
>  notmuch (0.8~rc1-1) experimental; urgency=low
>  
> +  [ David Bremner ]
>    * Upstream release candidate.
>  
> - -- David Bremner <bremner@debian.org>  Tue, 06 Sep 2011 22:24:24 -0300
> +  [ Martin Owens (DoctorMO) ]
> +  * Re-new-world-order
> +
> + -- Martin Owens (DoctorMO) <doctormo@gmail.com>  Wed, 07 Sep 2011 18:19:50 -0400
>  
>  notmuch (0.7-1) unstable; urgency=low
>  
> diff --git a/debian/control b/debian/control
> index 03afdf4..a72fe1b 100644
> --- 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),
>   emacs23-nox | emacs23 (>=23~) | emacs23-lucid (>=23~)
>  Standards-Version: 3.9.2
>  Homepage: http://notmuchmail.org/

Even if you found the above two changes necessary, they shouldn't be
included in this patch because they aren't related to reopen/flush.

> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -700,18 +700,36 @@ notmuch_database_open (const char *path,
>  }
>  
>  void
> -notmuch_database_close (notmuch_database_t *notmuch)
> +notmuch_database_reopen (notmuch_database_t *notmuch)
> +{
> +       try {
> +               notmuch->xapian_db->reopen ();
> +       } catch (const Xapian::Error &error) {
> +               if (! notmuch->exception_reported) {
> +               fprintf (stderr, "Error: A Xapian exception occurred reopening database: %s\n",
> +               error.get_msg().c_str());
> +               }
> +       }
> +}
> +
> +void
> +notmuch_database_flush (notmuch_database_t *notmuch)
>  {
>      try {
>         if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
> -           (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
> +               (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
>      } catch (const Xapian::Error &error) {
> -       if (! notmuch->exception_reported) {
> -           fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
> -                    error.get_msg().c_str());
> +               if (! notmuch->exception_reported) {
> +               fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
> +               error.get_msg().c_str());
> +               }
>         }
> -    }
> +}

Both catch blocks should set notmuch->exception_reported = TRUE;
(notmuch_database_close happens to be the one exception to this since
it's closing the database; see other catch blocks in lib/database.cc).

The catch block in notmuch_database_flush shouldn't appear as changed
in the diff; you should leave the spacing the way it originally was.

Re: Patch: Flush and Reopen

[Martin Owens]

On Sun, 2011-09-11 at 20:23 -0400, Austin Clements wrote:
> BTW, in the future, you should send patches inline (see the patch
> formatting guide I linked to earlier for easy ways to do this).  It
> makes them much easier to review and reply to.

I tried to do this, my email client doesn't allow this sort of mucking
about. sorry guys bare with the patch file.

> I assume you've tested it?

Absolutely, I have my testing framework and it would fail quite badly if
either flush or reopen failed to work in either py binding or lib.

> Notmuch is quite consistent about indentation, though the rule may not
> be something you'd expect coming from Python.  It uses four space
> indentation, but each group of eight spaces is replaced with a tab, so
> for example

I see, I'm going to forgo the rage and just do it.

> Even if you found the above two changes necessary, they shouldn't be
> included in this patch because they aren't related to reopen/flush.

There were intended to be removed. Should be gone now.

> Both catch blocks should set notmuch->exception_reported = TRUE;
> (notmuch_database_close happens to be the one exception to this since
> it's closing the database; see other catch blocks in lib/database.cc).

Added.

> The catch block in notmuch_database_flush shouldn't appear as changed
> in the diff; you should leave the spacing the way it originally was. 

The spacing is mostly the same now, I removed a word from the error
message deliberately before I noticed it was in the original text. But
it shouldn't be too important.

Regards, Martin Owens

Re: Patch: Flush and Reopen

[Martin Owens]

[-- Attachment #1: Type: text/plain, Size: 48 bytes --]

Previous Email was sans-attachment. See below.


[-- Attachment #2: 0001-Add-flush-and-reopen-to-notmuch-database-python-bind.patch --]
[-- Type: text/x-patch, Size: 3556 bytes --]

From 2d247d56660689dee5ae58c252621f9343044dd8 Mon Sep 17 00:00:00 2001
From: Martin Owens <doctormo@gmail.com>
Date: Sun, 11 Sep 2011 20:51:35 -0400
Subject: [PATCH] Add flush and reopen to notmuch database + python bindings.

Flush and reopen methods in the libnotmuch and python bindings
allows concurent access to the database. Flush allows read_write
databases to commit their changes, forcing a flush to disk and
reopen allows a database to reread from the disk when changes
are detected or known about.
---
 bindings/python/notmuch/database.py |   10 ++++++++++
 lib/database.cc                     |   28 ++++++++++++++++++++++++----
 lib/notmuch.h                       |    8 ++++++++
 3 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/bindings/python/notmuch/database.py b/bindings/python/notmuch/database.py
index f18ca14..b6c1c31 100644
--- a/bindings/python/notmuch/database.py
+++ b/bindings/python/notmuch/database.py
@@ -432,6 +432,16 @@ class Database(object):
 
         return Query(self, querystring)
 
+    def reopen(self):
+        """Reopens a read only database, when the data has changed, this is required."""
+        if self._db is not None:
+            nmlib.notmuch_database_reopen(self._db)
+
+    def flush(self):
+        """Flushes the search database, only available when in read-write."""
+        if self._db is not None:
+            nmlib.notmuch_database_flush(self._db)
+
     def __repr__(self):
         return "'Notmuch DB " + self.get_path() + "'"
 
diff --git a/lib/database.cc b/lib/database.cc
index 9c2f4ec..9258137 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -700,18 +700,38 @@ notmuch_database_open (const char *path,
 }
 
 void
-notmuch_database_close (notmuch_database_t *notmuch)
+notmuch_database_reopen (notmuch_database_t *notmuch)
+{
+    try {
+	notmuch->xapian_db->reopen ();
+    } catch (const Xapian::Error &error) {
+	if (! notmuch->exception_reported) {
+	    fprintf (stderr, "Error: A Xapian exception reopening database: %s\n",
+	    error.get_msg().c_str());
+	    notmuch->exception_reported = TRUE;
+	}
+    }
+}
+
+void
+notmuch_database_flush (notmuch_database_t *notmuch)
 {
     try {
 	if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
 	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
-    } catch (const Xapian::Error &error) {
+	} catch (const Xapian::Error &error) {
 	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
+	    fprintf (stderr, "Error: A Xapian exception flushing database: %s\n",
+	    error.get_msg().c_str());
+	    notmuch->exception_reported = TRUE;
 	}
     }
+}
 
+void
+notmuch_database_close (notmuch_database_t *notmuch)
+{
+    notmuch_database_flush (notmuch);
     delete notmuch->term_gen;
     delete notmuch->query_parser;
     delete notmuch->xapian_db;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 974be8d..ca87b89 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -171,6 +171,14 @@ notmuch_database_t *
 notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode);
 
+/* Reopen a read_only database when the data source has changed */
+void
+notmuch_database_reopen (notmuch_database_t *database);
+
+/* Force a flush of the xapian database, also done on close */
+void
+notmuch_database_flush (notmuch_database_t *database);
+
 /* Close the given notmuch database, freeing all associated
  * resources. See notmuch_database_open. */
 void
-- 
1.7.1

Re: Patch: Flush and Reopen

[Sebastian Spaeth]

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]

On Sun, 11 Sep 2011 21:04:39 -0400, Martin Owens <doctormo@gmail.com> wrote:

> Subject: [PATCH] Add flush and reopen to notmuch database + python bindings.

The python parts look fine to me, they certainly have my blessing to go
in if the rest is being checked in.

Sebastian

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]