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

[David Bremner]

Austin Clements  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

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

[Austin Clements]

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.  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  (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

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

[Austin Clements]

From: 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 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  
(notmuch->xapian_db))->flush ();
-} catch (const Xapian::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  (notmuch->xapian_db))
+   ->cancel_transaction ();
+
+   /* Close the database.  This implicitly flushes
+* outstanding changes. */
notmuch->xapian_db->close();
} catch (const Xapian::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

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

[W. Trevor King]

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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

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

[W. Trevor King]

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  (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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

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

[Austin Clements]

From: Austin Clements amdra...@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

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[W. Trevor King]

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


signature.asc
Description: OpenPGP digital signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[Austin Clements]

On Thu, 02 Oct 2014, W. Trevor King wk...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[W. Trevor King]

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


signature.asc
Description: OpenPGP digital signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch