[PATCH 0/2] Add flush/reopen methods to notmuch_database_t

[PATCH 0/2] Add flush/reopen methods to notmuch_database_t

[Adrien Bustany]

Xapian (and therefore Notmuch? I would not mind a confirmation here) is
thread safe as long as you don't share DB instances across threads. The
database backend supports one writer and several concurrent readers.
One possible model for multi threaded apps using Notmuch is to have one
thread with a writable notmuch_database_t, and various with read-only
notmuch_database_t instances. However, for the readers to see the
changes done by the writer, the writer needs to flush the changes to
disk, and the readers need to reopen the database.
Those two patches add two methods to notmuch_database_t for this
purpose (the signaling of the writer to the readers that they need to
reopen is left to the application).

Adrien Bustany (2):
  Add notmuch_database_flush method
  Add notmuch_database_reopen method

 lib/database.cc |   35 +++++++++++++++++++++++++++++++++++
 lib/notmuch.h   |   12 ++++++++++++
 2 files changed, 47 insertions(+), 0 deletions(-)

-- 
1.7.7.6

[PATCH 1/2] Add notmuch_database_flush method

[Adrien Bustany]

This method explicitly flushes the pending modifications to disk. It is
useful if your program has various threads, each with a read only DB and
one writer thread with a read/write DB. In that case, you most likely
want the writer to sync the changes to disk so that the readers can see
them, without having to close and reopen the database completely.
---
 lib/database.cc |   18 ++++++++++++++++++
 lib/notmuch.h   |    4 ++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 761dc1a..55bcd17 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -745,6 +745,24 @@ notmuch_database_open (const char *path,
     return status;
 }
 
+notmuch_status_t
+notmuch_database_flush(notmuch_database_t *notmuch)
+{
+	notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
+	try {
+	if (notmuch->xapian_db != NULL &&
+	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
+		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
+	} catch (const Xapian::Error &error) {
+	fprintf(stderr, "A Xapian exception occured flushing the database: %s\n",
+	        error.get_msg().c_str());
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	}
+
+	return status;
+}
+
 void
 notmuch_database_close (notmuch_database_t *notmuch)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3633bed..aef5c56 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -201,6 +201,10 @@ notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode,
 		       notmuch_database_t **database);
 
+/* Flushes all the pending modifications to the database to disk. */
+notmuch_status_t
+notmuch_database_flush (notmuch_database_t *database);
+
 /* Close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
-- 
1.7.7.6

Re: [PATCH 1/2] Add notmuch_database_flush method

[Ethan Glasser-Camp]

Adrien Bustany <adrien@bustany.org> writes:

> This method explicitly flushes the pending modifications to disk. It is
> useful if your program has various threads, each with a read only DB and
> one writer thread with a read/write DB. In that case, you most likely
> want the writer to sync the changes to disk so that the readers can see
> them, without having to close and reopen the database completely.

These patches are pretty straightforward. But to conform to notmuch style..

> +notmuch_status_t
> +notmuch_database_flush(notmuch_database_t *notmuch)
> +{
> +	notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;

Indent is 4 spaces. (You have tabs here, which are 8 spaces, according
to devel/STYLE.)

> +	try {
> +	if (notmuch->xapian_db != NULL &&

if should be more indented than try. (So when you pull try back to 4
spaces, leave if at 8 spaces.)

> +	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
> +		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();

This line is 90 characters, and will remain at 86 once you indent using
the in-house style. I'm not sure if it's worth reformatting.
notmuch_database_close calls flush() using exactly the same 86-character
line. I'd say "don't make it worse", but personally I think breaking
this line might be worse.

Ethan

Re: [PATCH 1/2] Add notmuch_database_flush method

[Adrien Bustany]

Le 17/10/2012 18:53, Ethan Glasser-Camp a écrit :
> Adrien Bustany <adrien@bustany.org> writes:
>
>> This method explicitly flushes the pending modifications to disk. It is
>> useful if your program has various threads, each with a read only DB and
>> one writer thread with a read/write DB. In that case, you most likely
>> want the writer to sync the changes to disk so that the readers can see
>> them, without having to close and reopen the database completely.
>
> These patches are pretty straightforward. But to conform to notmuch style..
>
>> +notmuch_status_t
>> +notmuch_database_flush(notmuch_database_t *notmuch)
>> +{
>> +	notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
>
> Indent is 4 spaces. (You have tabs here, which are 8 spaces, according
> to devel/STYLE.)
>
>> +	try {
>> +	if (notmuch->xapian_db != NULL &&
>
> if should be more indented than try. (So when you pull try back to 4
> spaces, leave if at 8 spaces.)

Sorry about that... I think I copied the style from 
notmuch_database_close, but my editor was set to have 4-space tabs, so I 
got confused. I'll send a fixed version of the patches.

>
>> +	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
>> +		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
>
> This line is 90 characters, and will remain at 86 once you indent using
> the in-house style. I'm not sure if it's worth reformatting.
> notmuch_database_close calls flush() using exactly the same 86-character
> line. I'd say "don't make it worse", but personally I think breaking
> this line might be worse.
>

Yes, I also think breaking this line will not make things prettier :-/

> Ethan
>

Thanks for the review!

Adrien

[PATCH 2/2] Add notmuch_database_reopen method

[Adrien Bustany]

Calling notmuch_database_reopen is needed to refresh the database
contents when the database on disk was modified by another
notmuch_database_t instance, for example in a different thread.
---
 lib/database.cc |   17 +++++++++++++++++
 lib/notmuch.h   |    8 ++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 55bcd17..3be5a30 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -763,6 +763,23 @@ notmuch_database_flush(notmuch_database_t *notmuch)
 	return status;
 }
 
+notmuch_status_t
+notmuch_database_reopen(notmuch_database_t *notmuch)
+{
+	notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
+	try {
+	if (notmuch->xapian_db != NULL)
+		(notmuch->xapian_db)->reopen ();
+	} catch (const Xapian::Error &error) {
+	fprintf(stderr, "A Xapian exception occured reopening the database: %s\n",
+	        error.get_msg().c_str());
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	}
+
+	return status;
+}
+
 void
 notmuch_database_close (notmuch_database_t *notmuch)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index aef5c56..51d6a9a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -205,6 +205,14 @@ notmuch_database_open (const char *path,
 notmuch_status_t
 notmuch_database_flush (notmuch_database_t *database);
 
+/* Refresh the database contents to the latest version.
+ *
+ * This is needed only if another instance of notmuch_database_t has
+ * modified the database contents on disk.
+ */
+notmuch_status_t
+notmuch_database_reopen (notmuch_database_t *database);
+
 /* Close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
-- 
1.7.7.6

[PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[Adrien Bustany]

The code of the patches in unchanged, but the formatting issues are now
hopefully fixed.

[PATCH 1/2] Add notmuch_database_flush method

[Adrien Bustany]

This method explicitly flushes the pending modifications to disk. It is
useful if your program has various threads, each with a read only DB and
one writer thread with a read/write DB. In that case, you most likely
want the writer to sync the changes to disk so that the readers can see
them, without having to close and reopen the database completely.
---
 lib/database.cc | 18 ++++++++++++++++++
 lib/notmuch.h   |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index 761dc1a..1b680ab 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -745,6 +745,24 @@ notmuch_database_open (const char *path,
     return status;
 }
 
+notmuch_status_t
+notmuch_database_flush(notmuch_database_t *notmuch)
+{
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
+    try {
+	if (notmuch->xapian_db != NULL &&
+	    notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE)
+	    (static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))->flush ();
+    } catch (const Xapian::Error &error) {
+	fprintf(stderr, "A Xapian exception occured flushing the database: %s\n",
+		error.get_msg().c_str());
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+
+    return status;
+}
+
 void
 notmuch_database_close (notmuch_database_t *notmuch)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3633bed..aef5c56 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -201,6 +201,10 @@ notmuch_database_open (const char *path,
 		       notmuch_database_mode_t mode,
 		       notmuch_database_t **database);
 
+/* Flushes all the pending modifications to the database to disk. */
+notmuch_status_t
+notmuch_database_flush (notmuch_database_t *database);
+
 /* Close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
-- 
1.7.11.7

[PATCH 2/2] Add notmuch_database_reopen method

[Adrien Bustany]

Calling notmuch_database_reopen is needed to refresh the database
contents when the database on disk was modified by another
notmuch_database_t instance, for example in a different thread.
---
 lib/database.cc | 17 +++++++++++++++++
 lib/notmuch.h   |  8 ++++++++
 2 files changed, 25 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index 1b680ab..f27d57f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -763,6 +763,23 @@ notmuch_database_flush(notmuch_database_t *notmuch)
     return status;
 }
 
+notmuch_status_t
+notmuch_database_reopen(notmuch_database_t *notmuch)
+{
+    notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
+
+    try {
+	if (notmuch->xapian_db != NULL)
+	    (notmuch->xapian_db)->reopen ();
+    } catch (const Xapian::Error &error) {
+	fprintf(stderr, "A Xapian exception occured reopening the database: %s\n",
+		error.get_msg().c_str());
+	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+    }
+
+    return status;
+}
+
 void
 notmuch_database_close (notmuch_database_t *notmuch)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index aef5c56..51d6a9a 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -205,6 +205,14 @@ notmuch_database_open (const char *path,
 notmuch_status_t
 notmuch_database_flush (notmuch_database_t *database);
 
+/* Refresh the database contents to the latest version.
+ *
+ * This is needed only if another instance of notmuch_database_t has
+ * modified the database contents on disk.
+ */
+notmuch_status_t
+notmuch_database_reopen (notmuch_database_t *database);
+
 /* Close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
-- 
1.7.11.7

Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[Ethan Glasser-Camp]

Adrien Bustany <adrien@bustany.org> writes:

> The code of the patches in unchanged, but the formatting issues are now
> hopefully fixed.

These look fine to me, and they're pretty trivial.

Ethan

Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[Jani Nikula]

On Wed, 17 Oct 2012, Adrien Bustany <adrien@bustany.org> wrote:
> The code of the patches in unchanged, but the formatting issues are now
> hopefully fixed.

Hi Adrien, please check at what version flush and reopen have been
introduced to xapian. If they are new-ish (I don't know, didn't have the
time to check), please add appropriate #ifdefs. [1] lays the groundwork
for this. We'll also need to decide what is the minimum xapian version
required in general, i.e. features earlier than that don't need
conditional compilation.

BR,
Jani.

[1] id:"1350487737-32058-2-git-send-email-bgamari.foss@gmail.com"

Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[Ethan Glasser-Camp]

Jani Nikula <jani@nikula.org> writes:

> On Wed, 17 Oct 2012, Adrien Bustany <adrien@bustany.org> wrote:
>> The code of the patches in unchanged, but the formatting issues are now
>> hopefully fixed.
>
> Hi Adrien, please check at what version flush and reopen have been
> introduced to xapian. If they are new-ish (I don't know, didn't have the
> time to check), please add appropriate #ifdefs. [1] lays the groundwork
> for this. We'll also need to decide what is the minimum xapian version
> required in general, i.e. features earlier than that don't need
> conditional compilation.

Hi! The new versions of these patches are still pretty trivial and they
still look OK to me, but based on Jani's prompting I decided to look up
the methods. Seems that flush() is a very old (pre-1.1.0, 2009-04) name
for commit(), which is the preferred name these days. You should
probably therefore rename the function notmuch_database_commit, and have
it call the WritableDatabase::commit() method.

reopen() is a very very old method, seems like it has been around since
2004.

So I think Adrien is safe from having to do version checks, but we
should probably use commit() instead of flush().

Ethan

Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[Adrien Bustany]

Le 20/10/2012 18:49, Ethan Glasser-Camp a écrit :
> Jani Nikula <jani@nikula.org> writes:
>
>> On Wed, 17 Oct 2012, Adrien Bustany <adrien@bustany.org> wrote:
>>> The code of the patches in unchanged, but the formatting issues are now
>>> hopefully fixed.
>>
>> Hi Adrien, please check at what version flush and reopen have been
>> introduced to xapian. If they are new-ish (I don't know, didn't have the
>> time to check), please add appropriate #ifdefs. [1] lays the groundwork
>> for this. We'll also need to decide what is the minimum xapian version
>> required in general, i.e. features earlier than that don't need
>> conditional compilation.
>
> Hi! The new versions of these patches are still pretty trivial and they
> still look OK to me, but based on Jani's prompting I decided to look up
> the methods. Seems that flush() is a very old (pre-1.1.0, 2009-04) name
> for commit(), which is the preferred name these days. You should
> probably therefore rename the function notmuch_database_commit, and have
> it call the WritableDatabase::commit() method.
>
> reopen() is a very very old method, seems like it has been around since
> 2004.
>
> So I think Adrien is safe from having to do version checks, but we
> should probably use commit() instead of flush().
>
> Ethan
>

Thanks for checking that! Sorry for the late answer, I had a hard time 
keeping on top of things lately...

Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[Michael Forney]

On Thu, 18 Oct 2012 00:52:20 +0300, Adrien Bustany <adrien@bustany.org> wrote:
> The code of the patches in unchanged, but the formatting issues are now
> hopefully fixed.

I would like to bump this patch set. I also need these features from
libnotmuch. Currently there is no way to recover from Xapian errors,
which is pretty limiting.

If it is the flush/commit that is the issue, I would be happy to make an
updated patch set.

-- 
Michael Forney <mforney@mforney.org>

Re: [PATCH v2 0/2] Add flush/reopen methods to notmuch_database_t

[David Bremner]

Michael Forney <mforney@mforney.org> writes:

> On Thu, 18 Oct 2012 00:52:20 +0300, Adrien Bustany <adrien@bustany.org> wrote:
>> The code of the patches in unchanged, but the formatting issues are now
>> hopefully fixed.
>
> I would like to bump this patch set. I also need these features from
> libnotmuch. Currently there is no way to recover from Xapian errors,
> which is pretty limiting.
>
> If it is the flush/commit that is the issue, I would be happy to make an
> updated patch set.

Hi Michael;

Right, we should use the current name of 'commit'. We also need some way
to check the xapian version in the configure script, and to make sure
the right name is supported.

Jani refers to 

     id:1350487737-32058-2-git-send-email-bgamari.foss@gmail.com

farther up the thread as having some relevant code/ideas.

I think it's reasonable to require Xapian version at least 1.2.0, so
that might simplify the configuration check. 

The other thing I wondered is if the error handling in these routines is
as good as it could be. I guess it isn't worse than what is there, but
especially since you are explicitly wanting to handle error conditions,
it would be good to think if we can do something better than adding yet
another printf to the library.

d