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

[David Bremner]

"W. Trevor King"  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

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

[David Bremner]

"W. Trevor King"  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

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

[David Bremner]

"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?


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

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

[W. Trevor King]

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

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

[W. Trevor King]

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

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

[W. Trevor King]

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

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

[David Bremner]

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

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

[David Bremner]

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

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

[W. Trevor King]

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


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

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

[David Bremner]

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

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

[W. Trevor King]

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


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

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

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

[W. Trevor King]

On Mon, Sep 22, 2014 at 06:50:50PM +, 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
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[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  
(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. */
+ * 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 ) {
-   /* 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

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

[W. Trevor King]

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

[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

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

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

[W. Trevor King]

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


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

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.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

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

[W. Trevor King]

On Mon, Sep 22, 2014 at 06:50:50PM +, 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


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