DatabaseModifiedErrors causing troubles

DatabaseModifiedErrors causing troubles

[Gaute Hope]

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

Hi,

I've been working on an application that keeps a read-only handle on
the notmuch database open for a long time. In some cases when a new
message is added along with some renames of other messages using
'notmuch new' while the application is running I get an Xapian
exception: DatabaseModifiedError:

  A Xapian exception occurred performing query: The revision being
read has been discarded - you
  should call Xapian::Database::reopen() and retry the operation.

Which seems to be printed from: notmuch_query_search_threads ->
notmuch_query_search_messages:294.

I have not been able to make a smaller test case at the moment (this
happens with offlineimap updating an maildir and notmuch new run
afterwards + some tagging).

I can work around this by checking for a NULL pointer returned from
notmuch_query_search_threads () and re-open the database
(notmuch_database_close () -> notmuch_database_open ()). But I have no
way of knowing programatically if this really is the error that has
happened. There should be some way of propagating the error
information or (even better for my case; for notmuch to reopen the
database), one option is the Gmime way of passing an pointer to an
error structure that is filled up by the notmuch interface function.

I made some attempts at exposing the ::reopen() function as suggested
by Xapian (http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#af140b1f8d948d13cf7be4a11a7c699a4),
but I end up with other errors afterwards. Possibly from leftover
structures created with the original database handle:

after notmuch_database_reopen (see attached patch for your reference):
  A Xapian exception occurred when reading header: Expected block
24615 to be level 1, not 0
  A Xapian exception occurred when reading header: Error reading block
419480589: got end of file
  A Xapian exception occurred when reading date: Error reading block
419480589: got end of file
  A Xapian exception occurred when reading header: Error reading block
419480589: got end of file


as mentioned, doing a manual _close and _open works. Again, the best
would be a consistent way to really know that this (or something else)
is the error that really happened.

Cheers, Gaute

[-- Attachment #2: 0001-lib-expose-XapianDatabae-reopen-through-notmuch_data.patch --]
[-- Type: text/x-patch, Size: 2142 bytes --]

From 90340fe59d677c989352f08e82f908016c25fafa Mon Sep 17 00:00:00 2001
From: Gaute Hope <eg@gaute.vetsj.com>
Date: Mon, 11 Aug 2014 14:16:12 +0200
Subject: [PATCH] lib: expose XapianDatabae::reopen() through
 notmuch_database_reopen()

---
 lib/database.cc | 18 ++++++++++++++++++
 lib/notmuch.h   | 22 ++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index c760290..80af410 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -778,6 +778,24 @@ notmuch_database_open (const char *path,
 }
 
 notmuch_status_t
+notmuch_database_reopen (notmuch_database_t *notmuch)
+{
+  try {
+    notmuch->xapian_db->reopen ();
+
+    return NOTMUCH_STATUS_SUCCESS;
+
+  } catch (const Xapian::Error &error) {
+	  if (! notmuch->exception_reported) {
+	    fprintf (stderr, "Error: A Xapian exception occurred while reopening database: %s\n",
+		     error.get_msg().c_str());
+	  }
+
+    return NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+  }
+}
+
+notmuch_status_t
 notmuch_database_close (notmuch_database_t *notmuch)
 {
     notmuch_status_t status = NOTMUCH_STATUS_SUCCESS;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3c5ec98..00950af 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -277,6 +277,28 @@ notmuch_database_open (const char *path,
 		       notmuch_database_t **database);
 
 /**
+ * Reopen the given notmuch database.
+ *
+ * The underlying Xapian database will be re-opened to the latest
+ * available version. It can be used to make sure the latest results
+ * are returned or to recover from an Xapaian::DatabaseModifiedError
+ * which can occur after external database modification.
+ *
+ * Calling notmuch_database_reopen on a database that has been closed
+ * will result in a NOTMUCH_STATUS_XAPAIAN_EXCEPTION.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successfully reopened database.
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred.
+ *
+ */
+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
-- 
2.0.3

Re: DatabaseModifiedErrors causing troubles

[Gaute Hope]

Gaute Hope <eg@gaute.vetsj.com> wrote on Mon, 11 Aug 2014 14:17:54 +0200:
> Hi,
>
> I've been working on an application that keeps a read-only handle on
> the notmuch database open for a long time. In some cases when a new
> message is added along with some renames of other messages using
> 'notmuch new' while the application is running I get an Xapian
> exception: DatabaseModifiedError:
>
>   A Xapian exception occurred performing query: The revision being
> read has been discarded - you
>   should call Xapian::Database::reopen() and retry the operation.
>
> Which seems to be printed from: notmuch_query_search_threads ->
> notmuch_query_search_messages:294.
>
> I have not been able to make a smaller test case at the moment (this
> happens with offlineimap updating an maildir and notmuch new run
> afterwards + some tagging).
>
> I can work around this by checking for a NULL pointer returned from
> notmuch_query_search_threads () and re-open the database
> (notmuch_database_close () -> notmuch_database_open ()). But I have no
> way of knowing programatically if this really is the error that has
> happened. There should be some way of propagating the error
> information or (even better for my case; for notmuch to reopen the
> database), one option is the Gmime way of passing an pointer to an
> error structure that is filled up by the notmuch interface function.

Hi again,

I have not yet been able to create a test for this with a fresh db (I
must be missing out on some of the parts triggering this).

I need to keep a query open for a while (because running it takes a
while), and this keeps coming back and biting me. I currently do a test
every time I need to access the query further (load more threads..). The
test is as follows, which is the easiest and quickest test I've found to
work reliably:


    /* tries to provoke an Xapian::DatabaseModifiedError
     *
     * returns true if db is invalid. queries will be invalid
     * afterwards, but might still respond true to notmuch_valid
     * calls.
     *
     */

    time_t start = clock ();
    bool invalid = false;

    /* testing */
    notmuch_query_t * q = notmuch_query_create (nm_db, "thread:fail");

    /* this will produce an error, but no way to ensure failure */
    notmuch_query_count_threads (q);

    /* this returns NULL when error */
    notmuch_threads_t * threads = notmuch_query_search_threads (q);

    if (threads == NULL) invalid = true;

    notmuch_query_destroy (q);

    float diff = (clock () - start) * 1000.0 / CLOCKS_PER_SEC;
    cout << "db: test query time: " << diff << " ms." << endl;

    if (invalid) {
      cout << "db: no longer valid, reopen required." << endl;
    } else {
      cout << "db: valid." << endl;
    }

As mentioned before, I am basically guessing that the reason I get
threads == NULL is the DatabaseModifiedError. There is no way to know
for sure.

For portability I would suggest starting to move towards the GError
scheme provided by glib (also used by gmime). This is a somewhat major
effort though since ensuring that error propagation is done right [0] is
somewhat tricky. It provides more or less the same functionality as
exceptions do in C++.

There is also the problem of having to change the API - one way to avoid
that is to create wrappers that behave like the old API, but don't
handle errors that good. This requires the addition of addiontal _error
variations of the current set of functions. It will be a mess.

Cheers, Gaute

[0] https://developer.gnome.org/glib/stable/glib-Error-Reporting.html

Re: DatabaseModifiedErrors causing troubles

[Gaute Hope]

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

On Thu, Aug 21, 2014 at 10:59 AM, Gaute Hope <eg@gaute.vetsj.com> wrote:
> For portability I would suggest starting to move towards the GError
> scheme provided by glib (also used by gmime). This is a somewhat major
> effort though since ensuring that error propagation is done right [0] is
> somewhat tricky. It provides more or less the same functionality as
> exceptions do in C++.
>
> There is also the problem of having to change the API - one way to avoid
> that is to create wrappers that behave like the old API, but don't
> handle errors that good. This requires the addition of addiontal _error
> variations of the current set of functions. It will be a mess.
>
> [0] https://developer.gnome.org/glib/stable/glib-Error-Reporting.html

Here's a quick mockup of how that could look.

Cheers, Gaute

[-- Attachment #2: 0001-mockup-Illustration-of-GError-for-error-reporting.patch --]
[-- Type: text/x-patch, Size: 4537 bytes --]

From eb49e311e312a5c9510ca24dec24ad1c60aedee9 Mon Sep 17 00:00:00 2001
From: Gaute Hope <eg@gaute.vetsj.com>
Date: Thu, 21 Aug 2014 10:56:57 +0200
Subject: [PATCH] mockup: Illustration of GError for error reporting

---
 lib/database.cc | 23 +++++++++++++++++++++--
 lib/notmuch.h   | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 9c0952a..085152f 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -42,6 +42,8 @@ typedef struct {
     const char *prefix;
 } prefix_t;
 
+GQuark notmuch_error_quark;
+
 #define NOTMUCH_DATABASE_VERSION 2
 
 #define STRINGIFY(s) _SUB_STRINGIFY(s)
@@ -367,16 +369,28 @@ _message_id_compressed (void *ctx, const char *message_id)
     return compressed;
 }
 
+/* deprecated wrapper */
 notmuch_status_t
 notmuch_database_find_message (notmuch_database_t *notmuch,
 			       const char *message_id,
 			       notmuch_message_t **message_ret)
 {
+  return notmuch_database_find_message_err (notmuch, message_id, message_ret, NULL);
+}
+
+notmuch_status_t
+notmuch_database_find_message_err (notmuch_database_t *notmuch,
+			       const char *message_id,
+			       notmuch_message_t **message_ret,
+             GError **err)
+{
     notmuch_private_status_t status;
     unsigned int doc_id;
 
-    if (message_ret == NULL)
-	return NOTMUCH_STATUS_NULL_POINTER;
+    if (message_ret == NULL) {
+      g_set_error (err, NOTMUCH_ERROR, NOTMUCH_STATUS_NULL_POINTER, notmuch_status_to_string (NOTMUCH_STATUS_NULL_POINTER));
+      return NOTMUCH_STATUS_NULL_POINTER;
+    }
 
     if (strlen (message_id) > NOTMUCH_MESSAGE_ID_MAX)
 	message_id = _message_id_compressed (notmuch, message_id);
@@ -633,6 +647,11 @@ notmuch_database_open (const char *path,
     unsigned int i, version;
     static int initialized = 0;
 
+    /* Initialize GError system */
+    if (! initialized) {
+      notmuch_error_quark = g_quark_from_static_string ("notmuch");
+    }
+
     if (path == NULL) {
 	fprintf (stderr, "Error: Cannot open a database for a NULL path.\n");
 	status = NOTMUCH_STATUS_NULL_POINTER;
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 3c5ec98..af28e09 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -42,6 +42,7 @@
 NOTMUCH_BEGIN_DECLS
 
 #include <time.h>
+#include <glib.h>
 
 #ifndef FALSE
 #define FALSE 0
@@ -159,6 +160,12 @@ typedef enum _notmuch_status {
      * The operation is not supported.
      */
     NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
+
+    /* An Xapian::DatabaseModifiedError has occurred, all decendants of
+     * database are invalid and the database must be reopened.
+     */
+    NOTMUCH_STATUS_XAPIAN_DATABASE_MODIFIED_ERROR,
+
     /**
      * Not an actual status value. Just a way to find out how many
      * valid status values there are.
@@ -166,6 +173,18 @@ typedef enum _notmuch_status {
     NOTMUCH_STATUS_LAST_STATUS
 } notmuch_status_t;
 
+
+/* errors */
+extern GQuark notmuch_error_quark;
+
+
+/**
+ * NOTMUCH_ERROR:
+ *
+ * The Notmuch error domain GQuark value.
+ **/
+#define NOTMUCH_ERROR notmuch_error_quark
+
 /**
  * Get a string representation of a notmuch_status_t value.
  *
@@ -554,6 +573,35 @@ notmuch_database_find_message (notmuch_database_t *database,
 			       notmuch_message_t **message);
 
 /**
+ * Find a message with the given message_id.
+ *
+ * If a message with the given message_id is found then, on successful return
+ * (NOTMUCH_STATUS_SUCCESS) '*message' will be initialized to a message
+ * object.  The caller should call notmuch_message_destroy when done with the
+ * message.
+ *
+ * On any failure or when the message is not found, this function initializes
+ * '*message' to NULL. This means, when NOTMUCH_STATUS_SUCCESS is returned, the
+ * caller is supposed to check '*message' for NULL to find out whether the
+ * message with the given message_id was found.
+ *
+ * Return value:
+ *
+ * NOTMUCH_STATUS_SUCCESS: Successful return, check '*message'.
+ *
+ * NOTMUCH_STATUS_NULL_POINTER: The given 'message' argument is NULL
+ *
+ * NOTMUCH_STATUS_OUT_OF_MEMORY: Out of memory, creating message object
+ *
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION: A Xapian exception occurred
+ */
+notmuch_status_t
+notmuch_database_find_message_err (notmuch_database_t *database,
+			       const char *message_id,
+			       notmuch_message_t **message,
+             GError **err);
+
+/**
  * Find a message with the given filename.
  *
  * If the database contains a message with the given filename then, on
-- 
2.0.4

Re: DatabaseModifiedErrors causing troubles

[David Bremner]

Gaute Hope <eg@gaute.vetsj.com> writes:

> I can work around this by checking for a NULL pointer returned from
> notmuch_query_search_threads () and re-open the database
> (notmuch_database_close () -> notmuch_database_open ()). But I have no
> way of knowing programatically if this really is the error that has
> happened. There should be some way of propagating the error
> information or (even better for my case; for notmuch to reopen the
> database), one option is the Gmime way of passing an pointer to an
> error structure that is filled up by the notmuch interface function.

Hi Gaute;

Sorry this sequence of postings of yours kindof fell down a well. In
general there seems to be not very much enthusiasm for the GError
solution.  We can do something less fancy with the series at

           id:1419788030-10567-2-git-send-email-david@tethera.net

In particular id:1419788030-10567-6-git-send-email-david@tethera.net
replaces the printfs with saving to a status string accessible from
notmuch_database_t.

I _think_ this could solve your problem, although doing strcmp on error
message might not be ideal. Overall this is much less api breakage
(only open and create need to be wrapped).

We could also consider really updating the api for those NULL returning
functions, but it seems less bad to me than the count functions updated
in this series

   id:1419971380-10307-2-git-send-email-david@tethera.net

Let me know what you think,

David

Re: DatabaseModifiedErrors causing troubles

[Gaute Hope]

Excerpts from David Bremner's message of December 31, 2014 9:28:
> Gaute Hope <eg@gaute.vetsj.com> writes:
> 
>> I can work around this by checking for a NULL pointer returned from
>> notmuch_query_search_threads () and re-open the database
>> (notmuch_database_close () -> notmuch_database_open ()). But I have no
>> way of knowing programatically if this really is the error that has
>> happened. There should be some way of propagating the error
>> information or (even better for my case; for notmuch to reopen the
>> database), one option is the Gmime way of passing an pointer to an
>> error structure that is filled up by the notmuch interface function.
> 
> Hi Gaute;
> 
> Sorry this sequence of postings of yours kindof fell down a well. In
> general there seems to be not very much enthusiasm for the GError
> solution.  We can do something less fancy with the series at
> 
>            id:1419788030-10567-2-git-send-email-david@tethera.net
> 
> In particular id:1419788030-10567-6-git-send-email-david@tethera.net
> replaces the printfs with saving to a status string accessible from
> notmuch_database_t.

Hi David,

Would it be possible with an error code that I could compare against in
stead? It would then work a bit like a global instance of the gmime
error. It could even be a preparation step against a gmime-error-style
solution in the far future.

I am sure you know all the bad reasons for using a strcmp with strings
such as small (subtle) changes making them useless or future
localization of notmuch. This solution is in my opinion worse than the
current situation, it will lock things in and create problems for future
API compatability and application maintainers. I would rather wait for
or spend some time on a more general solution.

Best regards, Gaute

> I _think_ this could solve your problem, although doing strcmp on error
> message might not be ideal. Overall this is much less api breakage
> (only open and create need to be wrapped).
> 
> We could also consider really updating the api for those NULL returning
> functions, but it seems less bad to me than the count functions updated
> in this series
> 
>    id:1419971380-10307-2-git-send-email-david@tethera.net
> 
> Let me know what you think,
> 
> David
> 

Re: DatabaseModifiedErrors causing troubles

[David Bremner]

Gaute Hope <eg@gaute.vetsj.com> writes:

>
> Hi David,
>
> Would it be possible with an error code that I could compare against in
> stead? It would then work a bit like a global instance of the gmime
> error. It could even be a preparation step against a gmime-error-style
> solution in the far future.
>
> I am sure you know all the bad reasons for using a strcmp with strings
> such as small (subtle) changes making them useless or future
> localization of notmuch. This solution is in my opinion worse than the
> current situation, it will lock things in and create problems for future
> API compatability and application maintainers. I would rather wait for
> or spend some time on a more general solution.

I don't agree it's worse than the current situation but I take your
point it isn't ideal.  We could do some kind "errno" in the database
structure.  I think there are not that many functions with this
unhelpful error return type. Based on a scan of notmuch.h, I see

notmuch_query_search_threads
notmuch_query_search_messages

and the two count functions that I already posted API breaking patches
for.  It might be better just to update the API (either adding versions
with error returns, or just forcing people to change) for these
functions.  Otherwise we have two different ways of returning status
codes, and the "errno" is only used some of the time.

d

Re: DatabaseModifiedErrors causing troubles

[Gaute Hope]

Excerpts from David Bremner's message of January 17, 2015 13:29:
> Gaute Hope <eg@gaute.vetsj.com> writes:
>
>>
>> Hi David,
>>
>> Would it be possible with an error code that I could compare against in
>> stead? It would then work a bit like a global instance of the gmime
>> error. It could even be a preparation step against a gmime-error-style
>> solution in the far future.
>>
>> I am sure you know all the bad reasons for using a strcmp with strings
>> such as small (subtle) changes making them useless or future
>> localization of notmuch. This solution is in my opinion worse than the
>> current situation, it will lock things in and create problems for future
>> API compatability and application maintainers. I would rather wait for
>> or spend some time on a more general solution.
>
> I don't agree it's worse than the current situation but I take your
> point it isn't ideal.  We could do some kind "errno" in the database
> structure.  I think there are not that many functions with this
> unhelpful error return type. Based on a scan of notmuch.h, I see
>
> notmuch_query_search_threads
> notmuch_query_search_messages
>
> and the two count functions that I already posted API breaking patches
> for.  It might be better just to update the API (either adding versions
> with error returns, or just forcing people to change) for these
> functions.  Otherwise we have two different ways of returning status
> codes, and the "errno" is only used some of the time.

Yeah - a consistent way of doing this would in my opinion be very
useful. Also, many other functions could be affected by outside
processes as well (notmuch new, notmuch tag) that do not necessarily
violate the thread-safety restrictions on xapian / notmuch. Errors in
these functions, and importantly which error, are currently hard to
catch and identify (say notmuch_messages_move_to_next). The same error
reporting system could be used for these. With a flexible error system
we could fix these as they are discovered.

Cheers, Gaute