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

2014-10-03 Thread David Bremner
Austin Clements acleme...@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
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch


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

2014-10-02 Thread 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

2014-10-02 Thread 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

2014-10-02 Thread 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