[PATCH] lib: Simplify close and codify aborting atomic section

[PATCH] lib: Simplify close and codify aborting atomic section

[Austin Clements]

In Xapian, closing a database implicitly aborts any outstanding
transaction and commits changes.  For historical reasons,
notmuch_database_close had grown to almost, but not quite duplicate
this behavior.  Before closing the database, it would explicitly (and
unnecessarily) commit it.  However, if there was an outstanding
transaction (ie atomic section), commit would throw a Xapian
exception, which notmuch_database_close would unnecessarily print to
stderr, even though notmuch_database_close would ultimately abort the
transaction anyway when it called close.

This patch simplifies notmuch_database_close to just call
Database::close.  This works for both read-only and read/write
databases, takes care of committing changes, unifies the exception
handling path, and codifies aborting outstanding transactions.  This
is currently the only way to abort an atomic section (and may remain
so, since it would be difficult to roll back things we may have cached
from rolled-back modifications).
---
 lib/database.cc | 23 +++++++----------------
 lib/notmuch.h   |  5 +++++
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index a3a7cd3..1f7ff2a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -903,28 +903,19 @@ notmuch_database_close (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) {
-	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
-	}
-    }
-
     /* Many Xapian objects (and thus notmuch objects) hold references to
      * the database, so merely deleting the database may not suffice to
-     * close it.  Thus, we explicitly close it here. */
+     * close it.  Thus, we explicitly close it here.  This will
+     * implicitly abort any outstanding transaction and commit changes. */
     if (notmuch->xapian_db != NULL) {
 	try {
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
-	    /* don't clobber previous error status */
-	    if (status == NOTMUCH_STATUS_SUCCESS)
-		status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    if (! notmuch->exception_reported) {
+		fprintf (stderr, "Error: A Xapian exception occurred closing database: %s\n",
+			 error.get_msg().c_str());
+	    }
 	}
     }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index fe2340b..5c40c67 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -292,6 +292,11 @@ notmuch_database_open (const char *path,
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
  *
+ * If the caller is currently in an atomic section (there was a
+ * notmuch_database_begin_atomic without a matching
+ * notmuch_database_end_atomic), this will abort the atomic section,
+ * discarding any modifications made in the atomic section.
+ *
  * Return value:
  *
  * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
-- 
2.1.0

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Mon, Sep 22, 2014 at 11:43:35AM -0400, Austin Clements wrote:
> This patch simplifies notmuch_database_close to just call
> Database::close.  This works for both read-only and read/write
> databases, takes care of committing changes, unifies the exception
> handling path, and codifies aborting outstanding transactions.

If we're dropping the flush call here, where will it be triggered
instead?  We'll need to flush/commit our changes to the database at
some point before closing.  Do clients now need an explicit
flush/commit command (explicit, client-initiated flushes sound like a
good idea to me).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[Austin Clements]

Quoth W. Trevor King on Sep 22 at  9:59 am:
> On Mon, Sep 22, 2014 at 11:43:35AM -0400, Austin Clements wrote:
> > This patch simplifies notmuch_database_close to just call
> > Database::close.  This works for both read-only and read/write
> > databases, takes care of committing changes, unifies the exception
> > handling path, and codifies aborting outstanding transactions.
> 
> If we're dropping the flush call here, where will it be triggered
> instead?  We'll need to flush/commit our changes to the database at
> some point before closing.  Do clients now need an explicit
> flush/commit command (explicit, client-initiated flushes sound like a
> good idea to me).

The call to Database::close implicitly flushes/commits, as mentioned
in the comment in the patch, so there's no need for any new APIs or
client changes.  The call to Database::flush in notmuch_database_close
was entirely redundant with the call to Database::close.

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Mon, Sep 22, 2014 at 06:50:50PM +0000, Austin Clements wrote:
> Quoth W. Trevor King on Sep 22 at  9:59 am:
> > On Mon, Sep 22, 2014 at 11:43:35AM -0400, Austin Clements wrote:
> > > This patch simplifies notmuch_database_close to just call
> > > Database::close.  This works for both read-only and read/write
> > > databases, takes care of committing changes, unifies the
> > > exception handling path, and codifies aborting outstanding
> > > transactions.
> > 
> > If we're dropping the flush call here, where will it be triggered
> > instead?  We'll need to flush/commit our changes to the database
> > at some point before closing.  Do clients now need an explicit
> > flush/commit command (explicit, client-initiated flushes sound
> > like a good idea to me).
> 
> The call to Database::close implicitly flushes/commits, as mentioned
> in the comment in the patch, so there's no need for any new APIs or
> client changes.  The call to Database::flush in
> notmuch_database_close was entirely redundant with the call to
> Database::close.

Ah, I thought the implicit flush/commit was just in our code.  Since
it's also in the underlying Xapian close, then this patch looks pretty
good to me.  I'd mention Xapian's explicit close in the notmuch.h
message.  Xapain's docs say [1]:

  For a WritableDatabase, if a transaction is active it will be
  aborted, while if no transaction is active commit() will be
  implicitly called.

Cheers,
Trevor

[1]: http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#a59f5f8b137723dcaaabdbdccbc0cf1eb

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:


> Ah, I thought the implicit flush/commit was just in our code.  Since
> it's also in the underlying Xapian close, then this patch looks pretty
> good to me.  I'd mention Xapian's explicit close in the notmuch.
h
> message.  Xapain's docs say [1]:
>
>   For a WritableDatabase, if a transaction is active it will be
>   aborted, while if no transaction is active commit() will be
>   implicitly called.

I'm not sure what you're asking for here by "explicit close". Isn't what
you quote a restatement of 

+ * If the caller is currently in an atomic section (there was a
+ * notmuch_database_begin_atomic without a matching
+ * notmuch_database_end_atomic), this will abort the atomic
section,
+ * discarding any modifications made in the atomic section.

in terms of underyling Xapian mechanics?


P.S. Other than whatever this doc question is, the patch looks ok to me.

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Wed, Sep 24, 2014 at 08:09:27PM +0200, David Bremner wrote:
> W. Trevor King writes:
> > Ah, I thought the implicit flush/commit was just in our code.
> > Since it's also in the underlying Xapian close, then this patch
> > looks pretty good to me.  I'd mention Xapian's explicit close in
> > the notmuch.h message.  Xapain's docs say [1]:
> >
> >   For a WritableDatabase, if a transaction is active it will be
> >   aborted, while if no transaction is active commit() will be
> >   implicitly called.
> 
> I'm not sure what you're asking for here by "explicit close". Isn't
> what you quote a restatement of
> 
> + * If the caller is currently in an atomic section (there was a
> + * notmuch_database_begin_atomic without a matching
> + * notmuch_database_end_atomic), this will abort the atomic section,
> + * discarding any modifications made in the atomic section.
> 
> in terms of underyling Xapian mechanics?

Sorry, I didn't phrase that very well.  The notmuch docs (as of this
patch) explain that we don't commit if we're in an atomic block.  The
Xapian docs also say that, *and* they say that if we're not in atomic
block the close *does* try to commit.  I think that's worth mentioning
in our close docs.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:

>
> Sorry, I didn't phrase that very well.  The notmuch docs (as of this
> patch) explain that we don't commit if we're in an atomic block.  The
> Xapian docs also say that, *and* they say that if we're not in atomic
> block the close *does* try to commit.  I think that's worth mentioning
> in our close docs.

OK, I see what you mean. I think the fact that you have to close the
notmuch database (when not using begin/end atomic) to get a commit is
surprising for many people, so it would be nice to make that clearer
somehow.

d

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Wed, Sep 24, 2014 at 09:25:20PM +0200, David Bremner wrote:
> I think the fact that you have to close the notmuch database (when
> not using begin/end atomic) to get a commit is surprising for many
> people, so it would be nice to make that clearer somehow.

It looks like Xapian is GPLv2+, so we should just be able to
copy/paste/edit the line I quoted from the Xapian docs.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[David Bremner]

"W. Trevor King" <wking@tremily.us> writes:

> On Wed, Sep 24, 2014 at 09:25:20PM +0200, David Bremner wrote:
>> I think the fact that you have to close the notmuch database (when
>> not using begin/end atomic) to get a commit is surprising for many
>> people, so it would be nice to make that clearer somehow.
>
> It looks like Xapian is GPLv2+, so we should just be able to
> copy/paste/edit the line I quoted from the Xapian docs.
>
> Cheers,
> Trevor

I think it would be better to write our own, not because of licensing
issues, but because the user of the notmuch API won't know what a xapian
commit is.

d

Re: [PATCH] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Wed, Sep 24, 2014 at 10:13:31PM +0200, David Bremner wrote:
> W. Trevor King writes:
> I think it would be better to write our own, not because of licensing
> issues, but because the user of the notmuch API won't know what a xapian
> commit is.

Between version control and databases, I feel like most folks using
libnotmuch will know what a commit is ;).  But I don't really mind, so
long as the new docs mention it for non-atomic closes.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2] lib: Simplify close and codify aborting atomic section

[Austin Clements]

From: Austin Clements <amdragon@mit.edu>

In Xapian, closing a database implicitly aborts any outstanding
transaction and commits changes.  For historical reasons,
notmuch_database_close had grown to almost, but not quite duplicate
this behavior.  Before closing the database, it would explicitly (and
unnecessarily) commit it.  However, if there was an outstanding
transaction (ie atomic section), commit would throw a Xapian
exception, which notmuch_database_close would unnecessarily print to
stderr, even though notmuch_database_close would ultimately abort the
transaction anyway when it called close.

This patch simplifies notmuch_database_close to just call
Database::close.  This works for both read-only and read/write
databases, takes care of committing changes, unifies the exception
handling path, and codifies aborting outstanding transactions.  This
is currently the only way to abort an atomic section (and may remain
so, since it would be difficult to roll back things we may have cached
from rolled-back modifications).
---
 lib/database.cc | 23 +++++++----------------
 lib/notmuch.h   |  5 +++++
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index a3a7cd3..1f7ff2a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -903,28 +903,19 @@ notmuch_database_close (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) {
-	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
-	}
-    }
-
     /* Many Xapian objects (and thus notmuch objects) hold references to
      * the database, so merely deleting the database may not suffice to
-     * close it.  Thus, we explicitly close it here. */
+     * close it.  Thus, we explicitly close it here.  This will
+     * implicitly abort any outstanding transaction and commit changes. */
     if (notmuch->xapian_db != NULL) {
 	try {
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
-	    /* don't clobber previous error status */
-	    if (status == NOTMUCH_STATUS_SUCCESS)
-		status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    if (! notmuch->exception_reported) {
+		fprintf (stderr, "Error: A Xapian exception occurred closing database: %s\n",
+			 error.get_msg().c_str());
+	    }
 	}
     }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index fe2340b..5c40c67 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -292,6 +292,11 @@ notmuch_database_open (const char *path,
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
  *
+ * If the caller is currently in an atomic section (there was a
+ * notmuch_database_begin_atomic without a matching
+ * notmuch_database_end_atomic), this will abort the atomic section,
+ * discarding any modifications made in the atomic section.
+ *
  * Return value:
  *
  * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
-- 
2.1.0

Re: [PATCH v2] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Wed, Sep 24, 2014 at 05:20:23PM -0400, Austin Clements wrote:
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index fe2340b..5c40c67 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -292,6 +292,11 @@ notmuch_database_open (const char *path,
>   * notmuch_database_close can be called multiple times.  Later calls
>   * have no effect.
>   *
> + * If the caller is currently in an atomic section (there was a
> + * notmuch_database_begin_atomic without a matching
> + * notmuch_database_end_atomic), this will abort the atomic section,
> + * discarding any modifications made in the atomic section.
> + *
>   * Return value:
>   *
>   * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.

Still no mention of “commit” or “if you're not in an atomic section”
;).

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3] lib: Simplify close and codify aborting atomic section

[Austin Clements]

From: Austin Clements <amdragon@mit.edu>

In Xapian, closing a database implicitly aborts any outstanding
transaction and commits changes.  For historical reasons,
notmuch_database_close had grown to almost, but not quite duplicate
this behavior.  Before closing the database, it would explicitly (and
unnecessarily) commit it.  However, if there was an outstanding
transaction (ie atomic section), commit would throw a Xapian
exception, which notmuch_database_close would unnecessarily print to
stderr, even though notmuch_database_close would ultimately abort the
transaction anyway when it called close.

This patch simplifies notmuch_database_close to just call
Database::close.  This works for both read-only and read/write
databases, takes care of committing changes, unifies the exception
handling path, and codifies aborting outstanding transactions.  This
is currently the only way to abort an atomic section (and may remain
so, since it would be difficult to roll back things we may have cached
from rolled-back modifications).
---

'Doh.  Left out the all-critical "git commit" before sending v2.

 lib/database.cc | 23 +++++++----------------
 lib/notmuch.h   |  8 +++++++-
 2 files changed, 14 insertions(+), 17 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index a3a7cd3..1f7ff2a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -903,28 +903,19 @@ notmuch_database_close (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) {
-	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
-	}
-    }
-
     /* Many Xapian objects (and thus notmuch objects) hold references to
      * the database, so merely deleting the database may not suffice to
-     * close it.  Thus, we explicitly close it here. */
+     * close it.  Thus, we explicitly close it here.  This will
+     * implicitly abort any outstanding transaction and commit changes. */
     if (notmuch->xapian_db != NULL) {
 	try {
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
-	    /* don't clobber previous error status */
-	    if (status == NOTMUCH_STATUS_SUCCESS)
-		status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    if (! notmuch->exception_reported) {
+		fprintf (stderr, "Error: A Xapian exception occurred closing database: %s\n",
+			 error.get_msg().c_str());
+	    }
 	}
     }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index fe2340b..49a3c79 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -281,7 +281,7 @@ notmuch_database_open (const char *path,
 		       notmuch_database_t **database);
 
 /**
- * Close the given notmuch database.
+ * Commit changes and close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
  * functions on objects derived from this database may either behave
@@ -292,6 +292,12 @@ notmuch_database_open (const char *path,
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
  *
+ * If the caller is currently in an atomic section (there was a
+ * notmuch_database_begin_atomic without a matching
+ * notmuch_database_end_atomic), this will abort the atomic section,
+ * discarding any modifications made in the atomic section.  All
+ * changes up to this will be committed.
+ *
  * Return value:
  *
  * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
-- 
2.1.0

Re: [PATCH v3] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Wed, Sep 24, 2014 at 05:32:50PM -0400, Austin Clements wrote:
> + * If the caller is currently in an atomic section (there was a
> + * notmuch_database_begin_atomic without a matching
> + * notmuch_database_end_atomic), this will abort the atomic section,
> + * discarding any modifications made in the atomic section.  All
> + * changes up to this will be committed.

I still think Xapian's wording is more readable [1]:

  For a WritableDatabase, if a transaction is active it will be
  aborted, while if no transaction is active commit() will be
  implicitly called.

How about:

  For a writable database, if a transaction is active (there was a
  notmuch_database_begin_atomic without a matching
  notmuch_database_end_atomic) it will be aborted, while if no
  transaction is active any pending changes will be committed.

Cheers,
Trevor

[1]: http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#a59f5f8b137723dcaaabdbdccbc0cf1eb

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3] lib: Simplify close and codify aborting atomic section

[Austin Clements]

On Wed, 24 Sep 2014, "W. Trevor King" <wking@tremily.us> wrote:
> On Wed, Sep 24, 2014 at 05:32:50PM -0400, Austin Clements wrote:
>> + * If the caller is currently in an atomic section (there was a
>> + * notmuch_database_begin_atomic without a matching
>> + * notmuch_database_end_atomic), this will abort the atomic section,
>> + * discarding any modifications made in the atomic section.  All
>> + * changes up to this will be committed.
>
> I still think Xapian's wording is more readable [1]:
>
>   For a WritableDatabase, if a transaction is active it will be
>   aborted, while if no transaction is active commit() will be
>   implicitly called.
>
> How about:
>
>   For a writable database, if a transaction is active (there was a
>   notmuch_database_begin_atomic without a matching
>   notmuch_database_end_atomic) it will be aborted, while if no
>   transaction is active any pending changes will be committed.

What is a "pending change" from the perspective of the notmuch API?
This is tricky because basically nothing in the library talks about
durability (partly because the notmuch API provides almost no control
over it).  Likewise, the API doesn't expose the notion of a transaction
(since that generally implies ACID), but only atomic sections.

I actually find the Xapian wording rather confusing.  Neither Xapian's
documentation nor your suggested comment say what happens when there is
*both* an outstanding transaction and pending changes.  In fact, teasing
this out made me realize that Xapian might in fact discard committed
(but unflushed) changes if you close the database with an outstanding
transaction.  But we definitely do want to flush these transactions
(especially since *all* of our atomic sections are "unflushed
transactions").  In v4 I've added some code to make sure this happens,
but because of the vagueness of the documentation I have no idea if it's
necessary.

> Cheers,
> Trevor
>
> [1]: http://xapian.org/docs/apidoc/html/classXapian_1_1Database.html#a59f5f8b137723dcaaabdbdccbc0cf1eb
>
> -- 
> This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
> For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[PATCH v4] lib: Simplify close and codify aborting atomic section

[Austin Clements]

From: Austin Clements <amdragon@mit.edu>

In Xapian, closing a database implicitly aborts any outstanding
transaction and commits changes.  For historical reasons,
notmuch_database_close had grown to almost, but not quite duplicate
this behavior.  Before closing the database, it would explicitly (and
unnecessarily) commit it.  However, if there was an outstanding
transaction (ie atomic section), commit would throw a Xapian
exception, which notmuch_database_close would unnecessarily print to
stderr, even though notmuch_database_close would ultimately abort the
transaction anyway when it called close.

This patch simplifies notmuch_database_close to explicitly abort any
outstanding transaction and then just call Database::close.  This
works for both read-only and read/write databases, takes care of
committing changes, unifies the exception handling path, and codifies
aborting outstanding transactions.  This is currently the only way to
abort an atomic section (and may remain so, since it would be
difficult to roll back things we may have cached from rolled-back
modifications).
---
 lib/database.cc | 32 +++++++++++++++++---------------
 lib/notmuch.h   |  9 ++++++++-
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index a3a7cd3..a47a71d 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -903,28 +903,30 @@ notmuch_database_close (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) {
-	status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
-	if (! notmuch->exception_reported) {
-	    fprintf (stderr, "Error: A Xapian exception occurred flushing database: %s\n",
-		     error.get_msg().c_str());
-	}
-    }
-
     /* Many Xapian objects (and thus notmuch objects) hold references to
      * the database, so merely deleting the database may not suffice to
      * close it.  Thus, we explicitly close it here. */
     if (notmuch->xapian_db != NULL) {
 	try {
+	    /* If there's an outstanding transaction, it's unclear if
+	     * closing the Xapian database commits everything up to
+	     * that transaction, or may discard committed (but
+	     * unflushed) transactions.  To be certain, explicitly
+	     * cancel any outstanding transaction before closing. */
+	    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
+		notmuch->atomic_nesting)
+		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))
+		    ->cancel_transaction ();
+
+	    /* Close the database.  This implicitly flushes
+	     * outstanding changes. */
 	    notmuch->xapian_db->close();
 	} catch (const Xapian::Error &error) {
-	    /* don't clobber previous error status */
-	    if (status == NOTMUCH_STATUS_SUCCESS)
-		status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    status = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	    if (! notmuch->exception_reported) {
+		fprintf (stderr, "Error: A Xapian exception occurred closing database: %s\n",
+			 error.get_msg().c_str());
+	    }
 	}
     }
 
diff --git a/lib/notmuch.h b/lib/notmuch.h
index fe2340b..dae0416 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -281,7 +281,7 @@ notmuch_database_open (const char *path,
 		       notmuch_database_t **database);
 
 /**
- * Close the given notmuch database.
+ * Commit changes and close the given notmuch database.
  *
  * After notmuch_database_close has been called, calls to other
  * functions on objects derived from this database may either behave
@@ -292,6 +292,13 @@ notmuch_database_open (const char *path,
  * notmuch_database_close can be called multiple times.  Later calls
  * have no effect.
  *
+ * For writable databases, notmuch_database_close commits all changes
+ * to disk before closing the database.  If the caller is currently in
+ * an atomic section (there was a notmuch_database_begin_atomic
+ * without a matching notmuch_database_end_atomic), this will discard
+ * changes made in that atomic section (but still commit changes made
+ * prior to entering the atomic section).
+ *
  * Return value:
  *
  * NOTMUCH_STATUS_SUCCESS: Successfully closed the database.
-- 
2.1.0

Re: [PATCH v4] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Thu, Oct 02, 2014 at 03:19:08PM -0400, Austin Clements wrote:
> This patch simplifies notmuch_database_close to explicitly abort any
> outstanding transaction and then just call Database::close.  This
> works for both read-only and read/write databases, takes care of
> committing changes, unifies the exception handling path, and codifies
> aborting outstanding transactions.

I don't expect atomic blocks are particularly useful for read-only
connections.  If they aren't, I'd quibble with the “This works for
both read-only…” wording above.  If they are, I'd drop the read/write
check below:

> +	    /* If there's an outstanding transaction, it's unclear if
> +	     * closing the Xapian database commits everything up to
> +	     * that transaction, or may discard committed (but
> +	     * unflushed) transactions.  To be certain, explicitly
> +	     * cancel any outstanding transaction before closing. */
> +	    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
> +		notmuch->atomic_nesting)
> +		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))
> +		    ->cancel_transaction ();

Thats a very minor quibble though, and I'd be glad to see this patch
land as is.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4] lib: Simplify close and codify aborting atomic section

[Austin Clements]

On Thu, 02 Oct 2014, "W. Trevor King" <wking@tremily.us> wrote:
> On Thu, Oct 02, 2014 at 03:19:08PM -0400, Austin Clements wrote:
>> This patch simplifies notmuch_database_close to explicitly abort any
>> outstanding transaction and then just call Database::close.  This
>> works for both read-only and read/write databases, takes care of
>> committing changes, unifies the exception handling path, and codifies
>> aborting outstanding transactions.
>
> I don't expect atomic blocks are particularly useful for read-only
> connections.  If they aren't, I'd quibble with the “This works for
> both read-only…” wording above.  If they are, I'd drop the read/write

It's true that atomic sections aren't very useful on a read-only
database, but we do allow them for symmetry.  We also keep track of how
deeply nested we are so notmuch_database_end_atomic can return a proper
error message regardless of whether the database is read/write or
read-only.

But all I'm saying in the commit message is that Xapian::Database::close
works for both read-only and read/write databases and will flush if
necessary, so we don't have to worry about that.

> check below:
>
>> +	    /* If there's an outstanding transaction, it's unclear if
>> +	     * closing the Xapian database commits everything up to
>> +	     * that transaction, or may discard committed (but
>> +	     * unflushed) transactions.  To be certain, explicitly
>> +	     * cancel any outstanding transaction before closing. */
>> +	    if (notmuch->mode == NOTMUCH_DATABASE_MODE_READ_WRITE &&
>> +		notmuch->atomic_nesting)
>> +		(static_cast <Xapian::WritableDatabase *> (notmuch->xapian_db))
>> +		    ->cancel_transaction ();

The notmuch->mode check here is necessary because atomic_nesting can be
non-zero on a read-only database (for the reason I mentioned above), but
we had better not cast xapian_db to a WritableDatabase if it isn't a
WritableDatabase.

> Thats a very minor quibble though, and I'd be glad to see this patch
> land as is.
>
> Cheers,
> Trevor
>
> -- 
> This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
> For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Re: [PATCH v4] lib: Simplify close and codify aborting atomic section

[W. Trevor King]

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

On Thu, Oct 02, 2014 at 04:39:41PM -0400, Austin Clements wrote:
> On Thu, 02 Oct 2014, W. Trevor King wrote:
> > On Thu, Oct 02, 2014 at 03:19:08PM -0400, Austin Clements wrote:
> >> This patch simplifies notmuch_database_close to explicitly abort
> >> any outstanding transaction and then just call Database::close.
> >> This works for both read-only and read/write databases, takes
> >> care of committing changes, unifies the exception handling path,
> >> and codifies aborting outstanding transactions.
> >
> > I don't expect atomic blocks are particularly useful for read-only
> > connections.  If they aren't, I'd quibble with the “This works for
> > both read-only…” wording above.  If they are, I'd drop the
> > read/write
> 
> It's true that atomic sections aren't very useful on a read-only
> database, but we do allow them for symmetry.

Heh, and they're no-ops since the beginning in 957f1ba3 (lib: Add
notmuch_database_{begin,end}_atomic, 2011-01-29).  So both the commit
message and read/write check make sense.  Quibble retracted.

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4] lib: Simplify close and codify aborting atomic section

[David Bremner]

Austin Clements <aclements@csail.mit.edu> writes:

> This patch simplifies notmuch_database_close to explicitly abort any
> outstanding transaction and then just call Database::close.  This
> works for both read-only and read/write databases, takes care of
> committing changes, unifies the exception handling path, and codifies
> aborting outstanding transactions.  This is currently the only way to
> abort an atomic section (and may remain so, since it would be
> difficult to roll back things we may have cached from rolled-back
> modifications).

pushed

d