unofficial mirror of notmuch@notmuchmail.org
 help / color / mirror / code / Atom feed
* [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-08-24  2:55 Ben Gamari
@ 2013-08-24  2:55 ` Ben Gamari
  2013-09-01 15:43   ` Jani Nikula
  0 siblings, 1 reply; 13+ messages in thread
From: Ben Gamari @ 2013-08-24  2:55 UTC (permalink / raw)
  To: notmuch

This function uses Xapian's Compactor machinery to compact the notmuch
database. The compacted database is built in a temporary directory and
later moved into place while the original uncompacted database is
preserved.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 configure       | 27 +++++++++++++++--
 lib/database.cc | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/notmuch.h   | 15 ++++++++++
 3 files changed, 130 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 6166917..ee9e887 100755
--- a/configure
+++ b/configure
@@ -277,7 +277,8 @@ printf "Checking for Xapian development files... "
 have_xapian=0
 for xapian_config in ${XAPIAN_CONFIG}; do
     if ${xapian_config} --version > /dev/null 2>&1; then
-	printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')
+	xapian_version=$(${xapian_config} --version | sed -e 's/.* //')
+	printf "Yes (%s).\n" ${xapian_version}
 	have_xapian=1
 	xapian_cxxflags=$(${xapian_config} --cxxflags)
 	xapian_ldflags=$(${xapian_config} --libs)
@@ -289,6 +290,21 @@ if [ ${have_xapian} = "0" ]; then
     errors=$((errors + 1))
 fi
 
+# Compaction is only supported on Xapian > 1.2.6
+have_xapian_compact=0
+if [ ${have_xapian} = "1" ]; then
+    printf "Checking for Xapian compact support... "
+    case "${xapian_version}" in
+        0.*|1.[01].*|1.2.[0-5])
+            printf "No (only available with Xapian > 1.2.6).\n" ;;
+        [1-9]*.[0-9]*.[0-9]*)
+            have_xapian_compact=1
+            printf "Yes.\n" ;;
+        *)
+            printf "Unknown version.\n" ;;
+    esac
+fi
+
 printf "Checking for GMime development files... "
 have_gmime=0
 IFS=';'
@@ -729,6 +745,9 @@ HAVE_STRCASESTR = ${have_strcasestr}
 # build its own version)
 HAVE_STRSEP = ${have_strsep}
 
+# Whether the Xapian version in use supports compaction
+HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -787,13 +806,15 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
 		   -DHAVE_STRSEP=\$(HAVE_STRSEP)                         \\
 		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
-		   -DSTD_ASCTIME=\$(STD_ASCTIME)
+		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
+                   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
 		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
 		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
 		     -DHAVE_STRSEP=\$(HAVE_STRSEP)                       \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
-		     -DSTD_ASCTIME=\$(STD_ASCTIME)
+		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
+                     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
 EOF
diff --git a/lib/database.cc b/lib/database.cc
index 5cc0765..b71751d 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -268,6 +268,8 @@ notmuch_status_to_string (notmuch_status_t status)
 	return "Unbalanced number of calls to notmuch_message_freeze/thaw";
     case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
 	return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";
+    case NOTMUCH_STATUS_UNSUPPORTED_OPERATION:
+	return "Unsupported operation";
     default:
     case NOTMUCH_STATUS_LAST_STATUS:
 	return "Unknown error status value";
@@ -800,6 +802,95 @@ notmuch_database_close (notmuch_database_t *notmuch)
     notmuch->date_range_processor = NULL;
 }
 
+class NotmuchCompactor : public Xapian::Compactor
+{
+public:
+    virtual void
+    set_status (const std::string &table, const std::string &status)
+    {
+	if (status.length() == 0)
+	    printf ("compacting table %s:\n", table.c_str());
+	else
+	    printf ("     %s\n", status.c_str());
+    }
+};
+
+#if HAVE_XAPIAN_COMPACT
+notmuch_status_t
+notmuch_database_compact_close (notmuch_database_t *notmuch)
+{
+    void *local = talloc_new (NULL);
+    NotmuchCompactor compactor;
+    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian_path;
+    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+
+    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", notmuch->path, ".notmuch"))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    if (! (old_xapian_path = talloc_asprintf (local, "%s.old", xapian_path))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    try {
+	compactor.set_renumber(false);
+	compactor.add_source(xapian_path);
+	compactor.set_destdir(compact_xapian_path);
+	compactor.compact();
+
+	if (rename(xapian_path, old_xapian_path)) {
+	    fprintf (stderr, "Error moving old database out of the way\n");
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+
+	if (rename(compact_xapian_path, xapian_path)) {
+	    fprintf (stderr, "Error moving compacted database\n");
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+    } catch (Xapian::InvalidArgumentError e) {
+	fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());
+	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	goto DONE;
+    }
+
+    fprintf (stderr, "\n");
+    fprintf (stderr, "\n");
+    fprintf (stderr, "Old database has been moved to %s", old_xapian_path);
+    fprintf (stderr, "\n");
+    fprintf (stderr, "To delete run,\n");
+    fprintf (stderr, "\n");
+    fprintf (stderr, "    rm -R %s\n", old_xapian_path);
+    fprintf (stderr, "\n");
+
+    notmuch_database_close(notmuch);
+
+DONE:
+    talloc_free(local);
+    return ret;
+}
+#else
+notmuch_status_t
+notmuch_database_compact_close (unused (notmuch_database_t *notmuch))
+{
+    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
+    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
+}
+#endif
+
 void
 notmuch_database_destroy (notmuch_database_t *notmuch)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 998a4ae..e9abd90 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -101,6 +101,7 @@ typedef enum _notmuch_status {
     NOTMUCH_STATUS_TAG_TOO_LONG,
     NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
     NOTMUCH_STATUS_UNBALANCED_ATOMIC,
+    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
 
     NOTMUCH_STATUS_LAST_STATUS
 } notmuch_status_t;
@@ -215,6 +216,20 @@ notmuch_database_open (const char *path,
 void
 notmuch_database_close (notmuch_database_t *database);
 
+/* Close the given notmuch database and then compact it.
+ *
+ * After notmuch_database_close_compact has been called, calls to
+ * other functions on objects derived from this database may either
+ * behave as if the database had not been closed (e.g., if the
+ * required data has been cached) or may fail with a
+ * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
+ *
+ * notmuch_database_close_compact can be called multiple times.  Later
+ * calls have no effect.
+ */
+notmuch_status_t
+notmuch_database_compact_close (notmuch_database_t *notmuch);
+
 /* Destroy the notmuch database, closing it if necessary and freeing
 * all associated resources. */
 void
-- 
1.8.1.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-08-24  2:55 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
@ 2013-09-01 15:43   ` Jani Nikula
  2013-09-03 14:32     ` Ben Gamari
  0 siblings, 1 reply; 13+ messages in thread
From: Jani Nikula @ 2013-09-01 15:43 UTC (permalink / raw)
  To: Ben Gamari, notmuch

On Sat, 24 Aug 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:
> This function uses Xapian's Compactor machinery to compact the notmuch
> database. The compacted database is built in a temporary directory and
> later moved into place while the original uncompacted database is
> preserved.
>
> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
> ---
>  configure       | 27 +++++++++++++++--
>  lib/database.cc | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/notmuch.h   | 15 ++++++++++
>  3 files changed, 130 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index 6166917..ee9e887 100755
> --- a/configure
> +++ b/configure
> @@ -277,7 +277,8 @@ printf "Checking for Xapian development files... "
>  have_xapian=0
>  for xapian_config in ${XAPIAN_CONFIG}; do
>      if ${xapian_config} --version > /dev/null 2>&1; then
> -	printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')
> +	xapian_version=$(${xapian_config} --version | sed -e 's/.* //')
> +	printf "Yes (%s).\n" ${xapian_version}
>  	have_xapian=1
>  	xapian_cxxflags=$(${xapian_config} --cxxflags)
>  	xapian_ldflags=$(${xapian_config} --libs)
> @@ -289,6 +290,21 @@ if [ ${have_xapian} = "0" ]; then
>      errors=$((errors + 1))
>  fi
>  
> +# Compaction is only supported on Xapian > 1.2.6
> +have_xapian_compact=0
> +if [ ${have_xapian} = "1" ]; then
> +    printf "Checking for Xapian compact support... "
> +    case "${xapian_version}" in
> +        0.*|1.[01].*|1.2.[0-5])
> +            printf "No (only available with Xapian > 1.2.6).\n" ;;
> +        [1-9]*.[0-9]*.[0-9]*)
> +            have_xapian_compact=1
> +            printf "Yes.\n" ;;
> +        *)
> +            printf "Unknown version.\n" ;;
> +    esac
> +fi
> +
>  printf "Checking for GMime development files... "
>  have_gmime=0
>  IFS=';'
> @@ -729,6 +745,9 @@ HAVE_STRCASESTR = ${have_strcasestr}
>  # build its own version)
>  HAVE_STRSEP = ${have_strsep}
>  
> +# Whether the Xapian version in use supports compaction
> +HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
> +
>  # Whether the getpwuid_r function is standards-compliant
>  # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
>  # to enable the standards-compliant version -- needed for Solaris)
> @@ -787,13 +806,15 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
>  		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
>  		   -DHAVE_STRSEP=\$(HAVE_STRSEP)                         \\
>  		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
> -		   -DSTD_ASCTIME=\$(STD_ASCTIME)
> +		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
> +                   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
>  CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
>  		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
>  		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
>  		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
>  		     -DHAVE_STRSEP=\$(HAVE_STRSEP)                       \\
>  		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
> -		     -DSTD_ASCTIME=\$(STD_ASCTIME)
> +		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
> +                     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
>  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
>  EOF
> diff --git a/lib/database.cc b/lib/database.cc
> index 5cc0765..b71751d 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -268,6 +268,8 @@ notmuch_status_to_string (notmuch_status_t status)
>  	return "Unbalanced number of calls to notmuch_message_freeze/thaw";
>      case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
>  	return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";
> +    case NOTMUCH_STATUS_UNSUPPORTED_OPERATION:
> +	return "Unsupported operation";
>      default:
>      case NOTMUCH_STATUS_LAST_STATUS:
>  	return "Unknown error status value";
> @@ -800,6 +802,95 @@ notmuch_database_close (notmuch_database_t *notmuch)
>      notmuch->date_range_processor = NULL;
>  }
>  
> +class NotmuchCompactor : public Xapian::Compactor
> +{
> +public:
> +    virtual void
> +    set_status (const std::string &table, const std::string &status)
> +    {
> +	if (status.length() == 0)
> +	    printf ("compacting table %s:\n", table.c_str());
> +	else
> +	    printf ("     %s\n", status.c_str());
> +    }

We're trying to reduce the amount of prints directly from libnotmuch,
not increase. This applies here as well as below.

> +};
> +
> +#if HAVE_XAPIAN_COMPACT
> +notmuch_status_t
> +notmuch_database_compact_close (notmuch_database_t *notmuch)
> +{
> +    void *local = talloc_new (NULL);
> +    NotmuchCompactor compactor;
> +    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian_path;
> +    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> +
> +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", notmuch->path, ".notmuch"))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (old_xapian_path = talloc_asprintf (local, "%s.old", xapian_path))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    try {
> +	compactor.set_renumber(false);
> +	compactor.add_source(xapian_path);
> +	compactor.set_destdir(compact_xapian_path);
> +	compactor.compact();
> +
> +	if (rename(xapian_path, old_xapian_path)) {
> +	    fprintf (stderr, "Error moving old database out of the way\n");
> +	    ret = NOTMUCH_STATUS_FILE_ERROR;
> +	    goto DONE;
> +	}

This fails if old_xapian_path exists.

> +
> +	if (rename(compact_xapian_path, xapian_path)) {
> +	    fprintf (stderr, "Error moving compacted database\n");
> +	    ret = NOTMUCH_STATUS_FILE_ERROR;
> +	    goto DONE;
> +	}
> +    } catch (Xapian::InvalidArgumentError e) {
> +	fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());
> +	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> +	goto DONE;
> +    }
> +
> +    fprintf (stderr, "\n");
> +    fprintf (stderr, "\n");
> +    fprintf (stderr, "Old database has been moved to %s", old_xapian_path);
> +    fprintf (stderr, "\n");
> +    fprintf (stderr, "To delete run,\n");
> +    fprintf (stderr, "\n");
> +    fprintf (stderr, "    rm -R %s\n", old_xapian_path);
> +    fprintf (stderr, "\n");
> +
> +    notmuch_database_close(notmuch);
> +
> +DONE:
> +    talloc_free(local);

The database does not get closed on errors. If that's intentional, it
should be documented.

> +    return ret;
> +}
> +#else
> +notmuch_status_t
> +notmuch_database_compact_close (unused (notmuch_database_t *notmuch))
> +{
> +    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
> +    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
> +}
> +#endif
> +
>  void
>  notmuch_database_destroy (notmuch_database_t *notmuch)
>  {
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 998a4ae..e9abd90 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -101,6 +101,7 @@ typedef enum _notmuch_status {
>      NOTMUCH_STATUS_TAG_TOO_LONG,
>      NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
>      NOTMUCH_STATUS_UNBALANCED_ATOMIC,
> +    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
>  
>      NOTMUCH_STATUS_LAST_STATUS
>  } notmuch_status_t;
> @@ -215,6 +216,20 @@ notmuch_database_open (const char *path,
>  void
>  notmuch_database_close (notmuch_database_t *database);
>  
> +/* Close the given notmuch database and then compact it.

The implementation first compacts then closes.

> + * After notmuch_database_close_compact has been called, calls to
> + * other functions on objects derived from this database may either
> + * behave as if the database had not been closed (e.g., if the
> + * required data has been cached) or may fail with a
> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
> + *
> + * notmuch_database_close_compact can be called multiple times.  Later
> + * calls have no effect.

This is not true. The Xapian compactor does not require the database to
be open. It will happily open the database read-only and compact the
database again if database has been closed.

> + */
> +notmuch_status_t
> +notmuch_database_compact_close (notmuch_database_t *notmuch);

I'm afraid we really need to re-think the API.

I see that your CLI 'notmuch compact' command opens the database
read-write, I assume to ensure there are no other writers, so that stuff
doesn't get lost. However if you pass a read-write database that has
been modified, I believe the changes will get lost (as Xapian opens the
database read-only). We shouldn't let the API users shoot themselves in
the foot so easily.

I think I'd go for something like:

notmuch_status_t
notmuch_database_compact (const char *path);

or

notmuch_status_t
notmuch_database_compact (const char *path, const char *backup);

which would internally open the database as read-write to ensure no
modifications, compact, and close. If backup != NULL, it would save the
old database there (same mounted file system as the database is a fine
limitation), otherwise remove.

Even then I'm not completely sure what Xapian WritableDatabase will do
on close when the database has been switched underneath it. But moving
the database after it has been closed has a race condition too.


BR,
Jani.



> +
>  /* Destroy the notmuch database, closing it if necessary and freeing
>  * all associated resources. */
>  void
> -- 
> 1.8.1.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

-- 
Jani

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-09-01 15:43   ` Jani Nikula
@ 2013-09-03 14:32     ` Ben Gamari
  0 siblings, 0 replies; 13+ messages in thread
From: Ben Gamari @ 2013-09-03 14:32 UTC (permalink / raw)
  To: Jani Nikula, notmuch

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

Jani Nikula <jani@nikula.org> writes:

> On Sat, 24 Aug 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:
>> This function uses Xapian's Compactor machinery to compact the notmuch
>> database. The compacted database is built in a temporary directory and
>> later moved into place while the original uncompacted database is
>> preserved.
>>

snip

>>  
>> +class NotmuchCompactor : public Xapian::Compactor
>> +{
>> +public:
>> +    virtual void
>> +    set_status (const std::string &table, const std::string &status)
>> +    {
>> +	if (status.length() == 0)
>> +	    printf ("compacting table %s:\n", table.c_str());
>> +	else
>> +	    printf ("     %s\n", status.c_str());
>> +    }
>
> We're trying to reduce the amount of prints directly from libnotmuch,
> not increase. This applies here as well as below.
>
Fair enough. That being said, I think that status updates are fairly
important given that the compaction process can be rather long.  Would
the preferred interface be to provide notmuch_database_compact_close
with a progress callback?

>> +};
>> +
>> +#if HAVE_XAPIAN_COMPACT
>> +notmuch_status_t
>> +notmuch_database_compact_close (notmuch_database_t *notmuch)
>> +{
>> +    void *local = talloc_new (NULL);
>> +    NotmuchCompactor compactor;
>> +    char *notmuch_path, *xapian_path, *compact_xapian_path, *old_xapian_path;
>> +    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
>> +
>> +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", notmuch->path, ".notmuch"))) {
>> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
>> +	goto DONE;
>> +    }
>> +
>> +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
>> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
>> +	goto DONE;
>> +    }
>> +
>> +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
>> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
>> +	goto DONE;
>> +    }
>> +
>> +    if (! (old_xapian_path = talloc_asprintf (local, "%s.old", xapian_path))) {
>> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
>> +	goto DONE;
>> +    }
>> +
>> +    try {
>> +	compactor.set_renumber(false);
>> +	compactor.add_source(xapian_path);
>> +	compactor.set_destdir(compact_xapian_path);
>> +	compactor.compact();
>> +
>> +	if (rename(xapian_path, old_xapian_path)) {
>> +	    fprintf (stderr, "Error moving old database out of the way\n");
>> +	    ret = NOTMUCH_STATUS_FILE_ERROR;
>> +	    goto DONE;
>> +	}
>
> This fails if old_xapian_path exists.
>
Ouch, yes, you are right. I suspect the right way forward here will be
to check whether old_xapian_path exists before beginning compaction,
allowing the user to fix the situation before it fails after finishing
what might be a pretty long process.

>> +
>> +	if (rename(compact_xapian_path, xapian_path)) {
>> +	    fprintf (stderr, "Error moving compacted database\n");
>> +	    ret = NOTMUCH_STATUS_FILE_ERROR;
>> +	    goto DONE;
>> +	}
>> +    } catch (Xapian::InvalidArgumentError e) {
>> +	fprintf (stderr, "Error while compacting: %s", e.get_msg().c_str());
>> +	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>> +	goto DONE;
>> +    }
>> +
>> +    fprintf (stderr, "\n");
>> +    fprintf (stderr, "\n");
>> +    fprintf (stderr, "Old database has been moved to %s", old_xapian_path);
>> +    fprintf (stderr, "\n");
>> +    fprintf (stderr, "To delete run,\n");
>> +    fprintf (stderr, "\n");
>> +    fprintf (stderr, "    rm -R %s\n", old_xapian_path);
>> +    fprintf (stderr, "\n");
>> +
>> +    notmuch_database_close(notmuch);
>> +
>> +DONE:
>> +    talloc_free(local);
>
> The database does not get closed on errors. If that's intentional, it
> should be documented.
>
I had reasons for this but they have long fled my memory. Regardless of
what it does, this behavior should be documented. I'll take care of this.

>> +    return ret;
>> +}
>> +#else
>> +notmuch_status_t
>> +notmuch_database_compact_close (unused (notmuch_database_t *notmuch))
>> +{
>> +    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
>> +    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
>> +}
>> +#endif
>> +
>>  void
>>  notmuch_database_destroy (notmuch_database_t *notmuch)
>>  {
>> diff --git a/lib/notmuch.h b/lib/notmuch.h
>> index 998a4ae..e9abd90 100644
>> --- a/lib/notmuch.h
>> +++ b/lib/notmuch.h
>> @@ -101,6 +101,7 @@ typedef enum _notmuch_status {
>>      NOTMUCH_STATUS_TAG_TOO_LONG,
>>      NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
>>      NOTMUCH_STATUS_UNBALANCED_ATOMIC,
>> +    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
>>  
>>      NOTMUCH_STATUS_LAST_STATUS
>>  } notmuch_status_t;
>> @@ -215,6 +216,20 @@ notmuch_database_open (const char *path,
>>  void
>>  notmuch_database_close (notmuch_database_t *database);
>>  
>> +/* Close the given notmuch database and then compact it.
>
> The implementation first compacts then closes.
>

>> + * After notmuch_database_close_compact has been called, calls to
>> + * other functions on objects derived from this database may either
>> + * behave as if the database had not been closed (e.g., if the
>> + * required data has been cached) or may fail with a
>> + * NOTMUCH_STATUS_XAPIAN_EXCEPTION.
>> + *
>> + * notmuch_database_close_compact can be called multiple times.  Later
>> + * calls have no effect.
>
> This is not true. The Xapian compactor does not require the database to
> be open. It will happily open the database read-only and compact the
> database again if database has been closed.
>
>> + */
>> +notmuch_status_t
>> +notmuch_database_compact_close (notmuch_database_t *notmuch);
>
> I'm afraid we really need to re-think the API.
>
It seems you are right. When writing this interface it was clear that
there would be a number of opportunities for misuse. I was hoping by
combining compact and close some of these would be eliminated but
clearly this isn't enough.

> I see that your CLI 'notmuch compact' command opens the database
> read-write, I assume to ensure there are no other writers, so that stuff
> doesn't get lost. However if you pass a read-write database that has
> been modified, I believe the changes will get lost (as Xapian opens the
> database read-only). We shouldn't let the API users shoot themselves in
> the foot so easily.
>
That is correct; the read-write database was an attempt to force the
user to exclusively lock the database they are trying to compact. It
seems that things can go quite wrong[1] when a database is modified
during compaction.  There was a suggestion in that thread to add an
option to lock the database during compaction. Perhaps it might be worth
bringing this up again with Xapian upstream. I think we agree that it
would be a poor idea to merge compaction functionality without having a
mechanism for ensuring data integrity, especially since many users
invoke notmuch in a cron job.

> I think I'd go for something like:
>
> notmuch_status_t
> notmuch_database_compact (const char *path);
>
> or
>
> notmuch_status_t
> notmuch_database_compact (const char *path, const char *backup);
>
> which would internally open the database as read-write to ensure no
> modifications, compact, and close. If backup != NULL, it would save the
> old database there (same mounted file system as the database is a fine
> limitation), otherwise remove.
>
This sounds fine to me.

> Even then I'm not completely sure what Xapian WritableDatabase will do
> on close when the database has been switched underneath it. But moving
> the database after it has been closed has a race condition too.
>
Good points. Not sure what the least evil way about this is. Hopefully
Xapian's close operation really does just close file handles.

Cheers,

- Ben


[1] http://lists.xapian.org/pipermail/xapian-discuss/2011-July/008310.html

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] notmuch compact support (v4)
@ 2013-10-02 20:30 Ben Gamari
  2013-10-02 20:30 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Ben Gamari @ 2013-10-02 20:30 UTC (permalink / raw)
  To: notmuch


Here is yet another iteration of my "notmuch compact" patchset. It has been
rebased on top of master and the interface reworked as suggested by Jani.

Cheers,

- Ben

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-10-02 20:30 [PATCH] notmuch compact support (v4) Ben Gamari
@ 2013-10-02 20:30 ` Ben Gamari
  2013-10-10 15:09   ` Tomi Ollila
  2013-11-02 18:30   ` Jameson Graef Rollins
  2013-10-02 20:30 ` [PATCH 2/3] notmuch-compact: Initial commit of CLI Ben Gamari
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 13+ messages in thread
From: Ben Gamari @ 2013-10-02 20:30 UTC (permalink / raw)
  To: notmuch

This function uses Xapian's Compactor machinery to compact the notmuch
database. The compacted database is built in a temporary directory and
later moved into place while the original uncompacted database is
preserved.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 configure       |  27 ++++++++--
 lib/database.cc | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/notmuch.h   |  21 +++++++-
 3 files changed, 195 insertions(+), 4 deletions(-)

diff --git a/configure b/configure
index 6166917..1a8e939 100755
--- a/configure
+++ b/configure
@@ -277,7 +277,8 @@ printf "Checking for Xapian development files... "
 have_xapian=0
 for xapian_config in ${XAPIAN_CONFIG}; do
     if ${xapian_config} --version > /dev/null 2>&1; then
-	printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')
+	xapian_version=$(${xapian_config} --version | sed -e 's/.* //')
+	printf "Yes (%s).\n" ${xapian_version}
 	have_xapian=1
 	xapian_cxxflags=$(${xapian_config} --cxxflags)
 	xapian_ldflags=$(${xapian_config} --libs)
@@ -289,6 +290,21 @@ if [ ${have_xapian} = "0" ]; then
     errors=$((errors + 1))
 fi
 
+# Compaction is only supported on Xapian > 1.2.6
+have_xapian_compact=0
+if [ ${have_xapian} = "1" ]; then
+    printf "Checking for Xapian compaction support... "
+    case "${xapian_version}" in
+        0.*|1.[01].*|1.2.[0-5])
+            printf "No (only available with Xapian > 1.2.6).\n" ;;
+        [1-9]*.[0-9]*.[0-9]*)
+            have_xapian_compact=1
+            printf "Yes.\n" ;;
+        *)
+            printf "Unknown version.\n" ;;
+    esac
+fi
+
 printf "Checking for GMime development files... "
 have_gmime=0
 IFS=';'
@@ -729,6 +745,9 @@ HAVE_STRCASESTR = ${have_strcasestr}
 # build its own version)
 HAVE_STRSEP = ${have_strsep}
 
+# Whether the Xapian version in use supports compaction
+HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
+
 # Whether the getpwuid_r function is standards-compliant
 # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
 # to enable the standards-compliant version -- needed for Solaris)
@@ -787,13 +806,15 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
 		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
 		   -DHAVE_STRSEP=\$(HAVE_STRSEP)                         \\
 		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
-		   -DSTD_ASCTIME=\$(STD_ASCTIME)
+		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
+		   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
 CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
 		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
 		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
 		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
 		     -DHAVE_STRSEP=\$(HAVE_STRSEP)                       \\
 		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
-		     -DSTD_ASCTIME=\$(STD_ASCTIME)
+		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
+		     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
 CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
 EOF
diff --git a/lib/database.cc b/lib/database.cc
index bb4f180..06f1c0a 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -24,7 +24,9 @@
 #include <iostream>
 
 #include <sys/time.h>
+#include <sys/stat.h>
 #include <signal.h>
+#include <ftw.h>
 
 #include <glib.h> /* g_free, GPtrArray, GHashTable */
 #include <glib-object.h> /* g_type_init */
@@ -268,6 +270,8 @@ notmuch_status_to_string (notmuch_status_t status)
 	return "Unbalanced number of calls to notmuch_message_freeze/thaw";
     case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
 	return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";
+    case NOTMUCH_STATUS_UNSUPPORTED_OPERATION:
+	return "Unsupported operation";
     default:
     case NOTMUCH_STATUS_LAST_STATUS:
 	return "Unknown error status value";
@@ -800,6 +804,153 @@ notmuch_database_close (notmuch_database_t *notmuch)
     notmuch->date_range_processor = NULL;
 }
 
+#if HAVE_XAPIAN_COMPACT
+static int unlink_cb (const char *path,
+		      unused (const struct stat *sb),
+		      unused (int type),
+		      unused (struct FTW *ftw))
+{
+    return remove(path);
+}
+
+static int rmtree (const char *path)
+{
+    return nftw(path, unlink_cb, 64, FTW_DEPTH | FTW_PHYS);
+}
+
+class NotmuchCompactor : public Xapian::Compactor
+{
+    notmuch_compact_status_cb_t status_cb;
+
+public:
+    NotmuchCompactor(notmuch_compact_status_cb_t cb) : status_cb(cb) { }
+
+    virtual void
+    set_status (const std::string &table, const std::string &status)
+    {
+	char* msg;
+
+	if (status_cb == NULL)
+	    return;
+
+	if (status.length() == 0)
+	    msg = talloc_asprintf (NULL, "compacting table %s", table.c_str());
+	else
+	    msg = talloc_asprintf (NULL, "     %s", status.c_str());
+
+	if (msg == NULL) {
+	    return;
+	}
+
+	status_cb(msg);
+	talloc_free(msg);
+    }
+};
+
+/* Compacts the given database, optionally saving the original database
+ * in backup_path. Additionally, a callback function can be provided to
+ * give the user feedback on the progress of the (likely long-lived)
+ * compaction process.
+ *
+ * The backup path must point to a directory on the same volume as the
+ * original database. Passing a NULL backup_path will result in the
+ * uncompacted database being deleted after compaction has finished.
+ * Note that the database write lock will be held during the
+ * compaction process to protect data integrity.
+ */
+notmuch_status_t
+notmuch_database_compact (const char* path,
+			  const char* backup_path,
+			  notmuch_compact_status_cb_t status_cb)
+{
+    void *local = talloc_new (NULL);
+    NotmuchCompactor compactor(status_cb);
+    char *notmuch_path, *xapian_path, *compact_xapian_path;
+    char *old_xapian_path = NULL;
+    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
+    notmuch_database_t *notmuch = NULL;
+    struct stat statbuf;
+
+    ret = notmuch_database_open(path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    if (ret) {
+	goto DONE;
+    }
+
+    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
+	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	goto DONE;
+    }
+
+    if (backup_path != NULL) {
+	if (! (old_xapian_path = talloc_asprintf (local, "%s/xapian.old", backup_path))) {
+	    ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
+	    goto DONE;
+	}
+
+	if (stat(old_xapian_path, &statbuf) != -1) {
+	    fprintf (stderr, "Backup path already exists: %s\n", old_xapian_path);
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+	if (errno != ENOENT) {
+	    fprintf (stderr, "Unknown error while stat()ing backup path: %s\n",
+		     strerror(errno));
+	    goto DONE;
+	}
+    }
+
+    try {
+	compactor.set_renumber(false);
+	compactor.add_source(xapian_path);
+	compactor.set_destdir(compact_xapian_path);
+	compactor.compact();
+    } catch (Xapian::InvalidArgumentError e) {
+	fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str());
+	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
+	goto DONE;
+    }
+
+    if (old_xapian_path != NULL) {
+	if (rename(xapian_path, old_xapian_path)) {
+	    fprintf (stderr, "Error moving old database out of the way\n");
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+    } else {
+	rmtree(xapian_path);
+    }
+
+    if (rename(compact_xapian_path, xapian_path)) {
+	fprintf (stderr, "Error moving compacted database\n");
+	ret = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+
+    notmuch_database_close(notmuch);
+
+DONE:
+    talloc_free(local);
+    return ret;
+}
+#else
+notmuch_status_t
+notmuch_database_compact_close (unused (notmuch_database_t *notmuch))
+{
+    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
+    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
+}
+#endif
+
 void
 notmuch_database_destroy (notmuch_database_t *notmuch)
 {
diff --git a/lib/notmuch.h b/lib/notmuch.h
index 998a4ae..9dab555 100644
--- a/lib/notmuch.h
+++ b/lib/notmuch.h
@@ -101,6 +101,7 @@ typedef enum _notmuch_status {
     NOTMUCH_STATUS_TAG_TOO_LONG,
     NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
     NOTMUCH_STATUS_UNBALANCED_ATOMIC,
+    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
 
     NOTMUCH_STATUS_LAST_STATUS
 } notmuch_status_t;
@@ -215,8 +216,26 @@ notmuch_database_open (const char *path,
 void
 notmuch_database_close (notmuch_database_t *database);
 
+/* A callback invoked by notmuch_database_compact to notify the user
+ * of the progress of the compaction process.
+ */
+typedef void (*notmuch_compact_status_cb_t)(const char*);
+
+/* Compact a notmuch database, backing up the original database to the
+ * given path.
+ *
+ * The database will be opened with NOTMUCH_DATABASE_MODE_READ_WRITE
+ * during the compaction process to ensure no writes are made.
+ *
+ */
+notmuch_status_t
+notmuch_database_compact (const char* path,
+			  const char* backup_path,
+			  notmuch_compact_status_cb_t status_cb);
+
 /* Destroy the notmuch database, closing it if necessary and freeing
-* all associated resources. */
+ * all associated resources.
+ */
 void
 notmuch_database_destroy (notmuch_database_t *database);
 
-- 
1.8.1.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] notmuch-compact: Initial commit of CLI
  2013-10-02 20:30 [PATCH] notmuch compact support (v4) Ben Gamari
  2013-10-02 20:30 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
@ 2013-10-02 20:30 ` Ben Gamari
  2013-10-02 20:30 ` [PATCH 3/3] notmuch-compact: Add man page Ben Gamari
  2013-10-10 10:22 ` [PATCH] notmuch compact support (v4) David Bremner
  3 siblings, 0 replies; 13+ messages in thread
From: Ben Gamari @ 2013-10-02 20:30 UTC (permalink / raw)
  To: notmuch

Introduce the user command exposing the new compaction facility.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 Makefile.local    |  1 +
 notmuch-client.h  |  3 +++
 notmuch-compact.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 notmuch.c         |  2 ++
 4 files changed, 62 insertions(+)
 create mode 100644 notmuch-compact.c

diff --git a/Makefile.local b/Makefile.local
index b7cd266..0464a50 100644
--- a/Makefile.local
+++ b/Makefile.local
@@ -257,6 +257,7 @@ notmuch_client_srcs =		\
 	gmime-filter-reply.c	\
 	hooks.c			\
 	notmuch.c		\
+	notmuch-compact.c	\
 	notmuch-config.c	\
 	notmuch-count.c		\
 	notmuch-dump.c		\
diff --git a/notmuch-client.h b/notmuch-client.h
index afb0ddf..0bfa4da 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -204,6 +204,9 @@ notmuch_tag_command (notmuch_config_t *config, int argc, char *argv[]);
 int
 notmuch_config_command (notmuch_config_t *config, int argc, char *argv[]);
 
+int
+notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[]);
+
 const char *
 notmuch_time_relative_date (const void *ctx, time_t then);
 
diff --git a/notmuch-compact.c b/notmuch-compact.c
new file mode 100644
index 0000000..bfda40e
--- /dev/null
+++ b/notmuch-compact.c
@@ -0,0 +1,56 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2013 Ben Gamari
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see http://www.gnu.org/licenses/ .
+ *
+ * Author: Ben Gamari <bgamari.foss@gmail.com>
+ */
+
+#include "notmuch-client.h"
+
+void status_update_cb (const char *msg);
+
+void
+status_update_cb (const char *msg)
+{
+    printf("%s\n", msg);
+}
+
+int
+notmuch_compact_command (notmuch_config_t *config,
+			 unused (int argc),
+			 unused (char *argv[]))
+{
+    const char *path = notmuch_config_get_database_path (config);
+    const char *backup_path = path;
+    notmuch_status_t ret;
+
+    printf ("Compacting database...\n");
+    ret = notmuch_database_compact (path, backup_path, status_update_cb);
+    if (ret) {
+	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
+    } else {
+	printf ("\n");
+	printf ("\n");
+	printf ("The old database has been moved to %s/xapian.old", backup_path);
+	printf ("\n");
+	printf ("To delete run,\n");
+	printf ("\n");
+	printf ("    rm -R %s/xapian.old\n", backup_path);
+	printf ("\n");
+    }
+
+    return 0;
+}
diff --git a/notmuch.c b/notmuch.c
index 7300c21..8d303a1 100644
--- a/notmuch.c
+++ b/notmuch.c
@@ -60,6 +60,8 @@ static command_t commands[] = {
       "Create a plain-text dump of the tags for each message." },
     { "restore", notmuch_restore_command, FALSE,
       "Restore the tags from the given dump file (see 'dump')." },
+    { "compact", notmuch_compact_command, FALSE,
+      "Compact the notmuch database." },
     { "config", notmuch_config_command, FALSE,
       "Get or set settings in the notmuch configuration file." },
     { "help", notmuch_help_command, TRUE, /* create but don't save config */
-- 
1.8.1.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] notmuch-compact: Add man page
  2013-10-02 20:30 [PATCH] notmuch compact support (v4) Ben Gamari
  2013-10-02 20:30 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
  2013-10-02 20:30 ` [PATCH 2/3] notmuch-compact: Initial commit of CLI Ben Gamari
@ 2013-10-02 20:30 ` Ben Gamari
  2013-10-10 10:22 ` [PATCH] notmuch compact support (v4) David Bremner
  3 siblings, 0 replies; 13+ messages in thread
From: Ben Gamari @ 2013-10-02 20:30 UTC (permalink / raw)
  To: notmuch

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 man/Makefile.local         |  1 +
 man/man1/notmuch-compact.1 | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+)
 create mode 100644 man/man1/notmuch-compact.1

diff --git a/man/Makefile.local b/man/Makefile.local
index 216aaa0..57910b7 100644
--- a/man/Makefile.local
+++ b/man/Makefile.local
@@ -8,6 +8,7 @@ MAIN_PAGE := $(dir)/man1/notmuch.1
 
 MAN1 := \
 	$(MAIN_PAGE) \
+	$(dir)/man1/notmuch-compact.1 \
 	$(dir)/man1/notmuch-config.1 \
 	$(dir)/man1/notmuch-count.1 \
 	$(dir)/man1/notmuch-dump.1 \
diff --git a/man/man1/notmuch-compact.1 b/man/man1/notmuch-compact.1
new file mode 100644
index 0000000..1aeed22
--- /dev/null
+++ b/man/man1/notmuch-compact.1
@@ -0,0 +1,36 @@
+.TH NOTMUCH-COMPACT 1 2013-08-23 "Notmuch 0.16"
+.SH NAME
+notmuch-compact \- compact the notmuch database
+.SH SYNOPSIS
+
+.B notmuch compact
+
+.SH DESCRIPTION
+
+The
+.B compact
+command can be used to compact the notmuch database. This can both reduce
+the space required by the database and improve lookup performance.
+
+The compacted database is built in a temporary directory and is later
+moved into the place of the origin database. The original uncompacted
+database is preserved to be deleted by the user as desired.
+
+Note that the database write lock will be held during the compaction
+process (which may be quite long) to protect data integrity.
+
+.RE
+.SH ENVIRONMENT
+The following environment variables can be used to control the
+behavior of notmuch.
+.TP
+.B NOTMUCH_CONFIG
+Specifies the location of the notmuch configuration file. Notmuch will
+use ${HOME}/.notmuch\-config if this variable is not set.
+.SH SEE ALSO
+
+\fBnotmuch\fR(1), \fBnotmuch-count\fR(1), \fBnotmuch-dump\fR(1),
+\fBnotmuch-hooks\fR(5), \fBnotmuch-insert\fR(1), \fBnotmuch-new\fR(1),
+\fBnotmuch-reply\fR(1), \fBnotmuch-restore\fR(1), \fBnotmuch-search\fR(1),
+\fBnotmuch-search-terms\fR(7), \fBnotmuch-show\fR(1),
+\fBnotmuch-tag\fR(1)
-- 
1.8.1.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] notmuch compact support (v4)
  2013-10-02 20:30 [PATCH] notmuch compact support (v4) Ben Gamari
                   ` (2 preceding siblings ...)
  2013-10-02 20:30 ` [PATCH 3/3] notmuch-compact: Add man page Ben Gamari
@ 2013-10-10 10:22 ` David Bremner
  2013-10-10 12:01   ` Ben Gamari
  3 siblings, 1 reply; 13+ messages in thread
From: David Bremner @ 2013-10-10 10:22 UTC (permalink / raw)
  To: Ben Gamari, notmuch

Ben Gamari <bgamari.foss@gmail.com> writes:

> Here is yet another iteration of my "notmuch compact" patchset. It has been
> rebased on top of master and the interface reworked as suggested by Jani.
>

Sorry, first version of this reply didn't go to the list.

I haven't had time to do a review, but I did try them out, and they
seemed not to eat my database.

What about a test for the test suite? 

 d

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] notmuch compact support (v4)
  2013-10-10 10:22 ` [PATCH] notmuch compact support (v4) David Bremner
@ 2013-10-10 12:01   ` Ben Gamari
  0 siblings, 0 replies; 13+ messages in thread
From: Ben Gamari @ 2013-10-10 12:01 UTC (permalink / raw)
  To: David Bremner, notmuch

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

David Bremner <david@tethera.net> writes:

> Ben Gamari <bgamari.foss@gmail.com> writes:
>
>> Here is yet another iteration of my "notmuch compact" patchset. It has been
>> rebased on top of master and the interface reworked as suggested by Jani.
>>
>
> Sorry, first version of this reply didn't go to the list.
>
> I haven't had time to do a review, but I did try them out, and they
> seemed not to eat my database.
>
> What about a test for the test suite? 
>
I'll implement something along these lines and send a patch shortly.

Cheers,

- Ben

[-- Attachment #2: Type: application/pgp-signature, Size: 489 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-10-02 20:30 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
@ 2013-10-10 15:09   ` Tomi Ollila
  2013-10-11 15:15     ` David Bremner
  2013-10-20 17:02     ` Tomi Ollila
  2013-11-02 18:30   ` Jameson Graef Rollins
  1 sibling, 2 replies; 13+ messages in thread
From: Tomi Ollila @ 2013-10-10 15:09 UTC (permalink / raw)
  To: Ben Gamari, notmuch

On Wed, Oct 02 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:

> This function uses Xapian's Compactor machinery to compact the notmuch
> database. The compacted database is built in a temporary directory and
> later moved into place while the original uncompacted database is
> preserved.
>
> Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
> ---
>  configure       |  27 ++++++++--
>  lib/database.cc | 151 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/notmuch.h   |  21 +++++++-
>  3 files changed, 195 insertions(+), 4 deletions(-)
>
> diff --git a/configure b/configure
> index 6166917..1a8e939 100755
> --- a/configure
> +++ b/configure
> @@ -277,7 +277,8 @@ printf "Checking for Xapian development files... "
>  have_xapian=0
>  for xapian_config in ${XAPIAN_CONFIG}; do
>      if ${xapian_config} --version > /dev/null 2>&1; then
> -	printf "Yes (%s).\n" $(${xapian_config} --version | sed -e 's/.* //')
> +	xapian_version=$(${xapian_config} --version | sed -e 's/.* //')
> +	printf "Yes (%s).\n" ${xapian_version}
>  	have_xapian=1
>  	xapian_cxxflags=$(${xapian_config} --cxxflags)
>  	xapian_ldflags=$(${xapian_config} --libs)
> @@ -289,6 +290,21 @@ if [ ${have_xapian} = "0" ]; then
>      errors=$((errors + 1))
>  fi
>  
> +# Compaction is only supported on Xapian > 1.2.6
> +have_xapian_compact=0
> +if [ ${have_xapian} = "1" ]; then
> +    printf "Checking for Xapian compaction support... "
> +    case "${xapian_version}" in
> +        0.*|1.[01].*|1.2.[0-5])
> +            printf "No (only available with Xapian > 1.2.6).\n" ;;
> +        [1-9]*.[0-9]*.[0-9]*)
> +            have_xapian_compact=1
> +            printf "Yes.\n" ;;
> +        *)
> +            printf "Unknown version.\n" ;;
> +    esac
> +fi
> +
>  printf "Checking for GMime development files... "
>  have_gmime=0
>  IFS=';'
> @@ -729,6 +745,9 @@ HAVE_STRCASESTR = ${have_strcasestr}
>  # build its own version)
>  HAVE_STRSEP = ${have_strsep}
>  
> +# Whether the Xapian version in use supports compaction
> +HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
> +
>  # Whether the getpwuid_r function is standards-compliant
>  # (if not, then notmuch will #define _POSIX_PTHREAD_SEMANTICS
>  # to enable the standards-compliant version -- needed for Solaris)
> @@ -787,13 +806,15 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)      \\
>  		   -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)                 \\
>  		   -DHAVE_STRSEP=\$(HAVE_STRSEP)                         \\
>  		   -DSTD_GETPWUID=\$(STD_GETPWUID)                       \\
> -		   -DSTD_ASCTIME=\$(STD_ASCTIME)
> +		   -DSTD_ASCTIME=\$(STD_ASCTIME)                         \\
> +		   -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
>  CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS)    \\
>  		     \$(TALLOC_CFLAGS) -DHAVE_VALGRIND=\$(HAVE_VALGRIND) \\
>  		     \$(VALGRIND_CFLAGS) \$(XAPIAN_CXXFLAGS)             \\
>  		     -DHAVE_STRCASESTR=\$(HAVE_STRCASESTR)               \\
>  		     -DHAVE_STRSEP=\$(HAVE_STRSEP)                       \\
>  		     -DSTD_GETPWUID=\$(STD_GETPWUID)                     \\
> -		     -DSTD_ASCTIME=\$(STD_ASCTIME)
> +		     -DSTD_ASCTIME=\$(STD_ASCTIME)                       \\
> +		     -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)
>  CONFIGURE_LDFLAGS =  \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(XAPIAN_LDFLAGS)
>  EOF
> diff --git a/lib/database.cc b/lib/database.cc
> index bb4f180..06f1c0a 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -24,7 +24,9 @@
>  #include <iostream>
>  
>  #include <sys/time.h>
> +#include <sys/stat.h>
>  #include <signal.h>
> +#include <ftw.h>
>  
>  #include <glib.h> /* g_free, GPtrArray, GHashTable */
>  #include <glib-object.h> /* g_type_init */
> @@ -268,6 +270,8 @@ notmuch_status_to_string (notmuch_status_t status)
>  	return "Unbalanced number of calls to notmuch_message_freeze/thaw";
>      case NOTMUCH_STATUS_UNBALANCED_ATOMIC:
>  	return "Unbalanced number of calls to notmuch_database_begin_atomic/end_atomic";
> +    case NOTMUCH_STATUS_UNSUPPORTED_OPERATION:
> +	return "Unsupported operation";
>      default:
>      case NOTMUCH_STATUS_LAST_STATUS:
>  	return "Unknown error status value";
> @@ -800,6 +804,153 @@ notmuch_database_close (notmuch_database_t *notmuch)
>      notmuch->date_range_processor = NULL;
>  }
>  
> +#if HAVE_XAPIAN_COMPACT
> +static int unlink_cb (const char *path,
> +		      unused (const struct stat *sb),
> +		      unused (int type),
> +		      unused (struct FTW *ftw))
> +{
> +    return remove(path);
> +}
> +
> +static int rmtree (const char *path)
> +{
> +    return nftw(path, unlink_cb, 64, FTW_DEPTH | FTW_PHYS);
> +}
> +
> +class NotmuchCompactor : public Xapian::Compactor
> +{
> +    notmuch_compact_status_cb_t status_cb;
> +
> +public:
> +    NotmuchCompactor(notmuch_compact_status_cb_t cb) : status_cb(cb) { }
> +
> +    virtual void
> +    set_status (const std::string &table, const std::string &status)
> +    {
> +	char* msg;
> +
> +	if (status_cb == NULL)
> +	    return;
> +
> +	if (status.length() == 0)
> +	    msg = talloc_asprintf (NULL, "compacting table %s", table.c_str());
> +	else
> +	    msg = talloc_asprintf (NULL, "     %s", status.c_str());
> +
> +	if (msg == NULL) {
> +	    return;
> +	}
> +
> +	status_cb(msg);
> +	talloc_free(msg);
> +    }
> +};
> +
> +/* Compacts the given database, optionally saving the original database
> + * in backup_path. Additionally, a callback function can be provided to
> + * give the user feedback on the progress of the (likely long-lived)
> + * compaction process.
> + *
> + * The backup path must point to a directory on the same volume as the
> + * original database. Passing a NULL backup_path will result in the
> + * uncompacted database being deleted after compaction has finished.
> + * Note that the database write lock will be held during the
> + * compaction process to protect data integrity.
> + */
> +notmuch_status_t
> +notmuch_database_compact (const char* path,
> +			  const char* backup_path,
> +			  notmuch_compact_status_cb_t status_cb)
> +{
> +    void *local = talloc_new (NULL);
> +    NotmuchCompactor compactor(status_cb);
> +    char *notmuch_path, *xapian_path, *compact_xapian_path;
> +    char *old_xapian_path = NULL;
> +    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> +    notmuch_database_t *notmuch = NULL;
> +    struct stat statbuf;
> +
> +    ret = notmuch_database_open(path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
> +    if (ret) {
> +	goto DONE;
> +    }
> +
> +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (backup_path != NULL) {
> +	if (! (old_xapian_path = talloc_asprintf (local, "%s/xapian.old", backup_path))) {
> +	    ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	    goto DONE;
> +	}
> +
> +	if (stat(old_xapian_path, &statbuf) != -1) {
> +	    fprintf (stderr, "Backup path already exists: %s\n", old_xapian_path);
> +	    ret = NOTMUCH_STATUS_FILE_ERROR;
> +	    goto DONE;
> +	}
> +	if (errno != ENOENT) {
> +	    fprintf (stderr, "Unknown error while stat()ing backup path: %s\n",
> +		     strerror(errno));
> +	    goto DONE;
> +	}
> +    }
> +
> +    try {
> +	compactor.set_renumber(false);
> +	compactor.add_source(xapian_path);
> +	compactor.set_destdir(compact_xapian_path);
> +	compactor.compact();

From functionality point if view this looks safe to me. 
A followup patch could provide more information to the user
is any of the following attemts fail, e.g. 

- if removing old database out of the way how to remove the new
  compacted database which can be considered as garbage now -- or
  how to rename it (which is a bit dangerous due to potential races)

- if moving compacted database fails how to restore backup database...
  ... or how to move compacted database to where it was supposed to be
  moved so that database is usable...

... if the database is missing is new created from scratch, also in case
there already is .notmuch directory  ?

... should the database open try to open database from these alternative
names in case opening from original name fails ?

another, small change:

      case "${xapian_version}" in
-         0.*|1.[01].*|1.2.[0-5])
+         0.*|1.[01].*|1.2.[0-5]|1.2.[0-5][^0-9]*)
              printf "No (only available with Xapian > 1.2.6).\n" ;;

someone might do version like 1.2.4-abc but probably not 1.2.04 (nor 1.2a.4)

(side note: case $x in [^...]) works with bash (and ksh&zsh, but not with dash)


> +    } catch (Xapian::InvalidArgumentError e) {
> +	fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str());
> +	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
> +	goto DONE;
> +    }
> +
> +    if (old_xapian_path != NULL) {
> +	if (rename(xapian_path, old_xapian_path)) {
> +	    fprintf (stderr, "Error moving old database out of the way\n");
> +	    ret = NOTMUCH_STATUS_FILE_ERROR;
> +	    goto DONE;
> +	}
> +    } else {
> +	rmtree(xapian_path);
> +    }
> +
> +    if (rename(compact_xapian_path, xapian_path)) {
> +	fprintf (stderr, "Error moving compacted database\n");
> +	ret = NOTMUCH_STATUS_FILE_ERROR;
> +	goto DONE;
> +    }
> +
> +    notmuch_database_close(notmuch);
> +
> +DONE:
> +    talloc_free(local);
> +    return ret;
> +}
> +#else
> +notmuch_status_t
> +notmuch_database_compact_close (unused (notmuch_database_t *notmuch))
> +{
> +    fprintf (stderr, "notmuch was compiled against a xapian version lacking compaction support.\n");
> +    return NOTMUCH_STATUS_UNSUPPORTED_OPERATION;
> +}
> +#endif
> +
>  void
>  notmuch_database_destroy (notmuch_database_t *notmuch)
>  {
> diff --git a/lib/notmuch.h b/lib/notmuch.h
> index 998a4ae..9dab555 100644
> --- a/lib/notmuch.h
> +++ b/lib/notmuch.h
> @@ -101,6 +101,7 @@ typedef enum _notmuch_status {
>      NOTMUCH_STATUS_TAG_TOO_LONG,
>      NOTMUCH_STATUS_UNBALANCED_FREEZE_THAW,
>      NOTMUCH_STATUS_UNBALANCED_ATOMIC,
> +    NOTMUCH_STATUS_UNSUPPORTED_OPERATION,
>  
>      NOTMUCH_STATUS_LAST_STATUS
>  } notmuch_status_t;
> @@ -215,8 +216,26 @@ notmuch_database_open (const char *path,
>  void
>  notmuch_database_close (notmuch_database_t *database);
>  
> +/* A callback invoked by notmuch_database_compact to notify the user
> + * of the progress of the compaction process.
> + */
> +typedef void (*notmuch_compact_status_cb_t)(const char*);
> +
> +/* Compact a notmuch database, backing up the original database to the
> + * given path.
> + *
> + * The database will be opened with NOTMUCH_DATABASE_MODE_READ_WRITE
> + * during the compaction process to ensure no writes are made.
> + *
> + */
> +notmuch_status_t
> +notmuch_database_compact (const char* path,
> +			  const char* backup_path,
> +			  notmuch_compact_status_cb_t status_cb);
> +
>  /* Destroy the notmuch database, closing it if necessary and freeing
> -* all associated resources. */
> + * all associated resources.
> + */
>  void
>  notmuch_database_destroy (notmuch_database_t *database);
>  
> -- 
> 1.8.1.2
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-10-10 15:09   ` Tomi Ollila
@ 2013-10-11 15:15     ` David Bremner
  2013-10-20 17:02     ` Tomi Ollila
  1 sibling, 0 replies; 13+ messages in thread
From: David Bremner @ 2013-10-11 15:15 UTC (permalink / raw)
  To: Tomi Ollila, Ben Gamari, notmuch

Tomi Ollila <tomi.ollila@iki.fi> writes:
>
> From functionality point if view this looks safe to me. 
> A followup patch could provide more information to the user
> is any of the following attemts fail, e.g. 
>
> - if removing old database out of the way how to remove the new
>   compacted database which can be considered as garbage now -- or
>   how to rename it (which is a bit dangerous due to potential races)
>
> - if moving compacted database fails how to restore backup database...
>   ... or how to move compacted database to where it was supposed to be
>   moved so that database is usable...
>
> ... if the database is missing is new created from scratch, also in case
> there already is .notmuch directory  ?
>
> ... should the database open try to open database from these alternative
> names in case opening from original name fails ?
>
> another, small change:
>
>       case "${xapian_version}" in
> -         0.*|1.[01].*|1.2.[0-5])
> +         0.*|1.[01].*|1.2.[0-5]|1.2.[0-5][^0-9]*)
>               printf "No (only available with Xapian > 1.2.6).\n" ;;
>
> someone might do version like 1.2.4-abc but probably not 1.2.04 (nor 1.2a.4)
>
> (side note: case $x in [^...]) works with bash (and ksh&zsh, but not with dash)

OK, let's leave those merged, and clean up these issues before the next
release.

d

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-10-10 15:09   ` Tomi Ollila
  2013-10-11 15:15     ` David Bremner
@ 2013-10-20 17:02     ` Tomi Ollila
  1 sibling, 0 replies; 13+ messages in thread
From: Tomi Ollila @ 2013-10-20 17:02 UTC (permalink / raw)
  To: Ben Gamari, notmuch

On Thu, Oct 10 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:

>
> another, small change:
>
>       case "${xapian_version}" in
> -         0.*|1.[01].*|1.2.[0-5])
> +         0.*|1.[01].*|1.2.[0-5]|1.2.[0-5][^0-9]*)
>               printf "No (only available with Xapian > 1.2.6).\n" ;;

Forget the above suggestion, as mentioned below this would not work in
dash. I presume attempting to compile compaction support will fail in 
case such support is not there. If we ever encounter such a bug report
let's revisit this then...

> someone might do version like 1.2.4-abc but probably not 1.2.04 (nor 1.2a.4)
>
> (side note: case $x in [^...]) works with bash (and ksh&zsh, but not with dash)

Tomi

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] database: Add notmuch_database_compact_close
  2013-10-02 20:30 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
  2013-10-10 15:09   ` Tomi Ollila
@ 2013-11-02 18:30   ` Jameson Graef Rollins
  1 sibling, 0 replies; 13+ messages in thread
From: Jameson Graef Rollins @ 2013-11-02 18:30 UTC (permalink / raw)
  To: Ben Gamari, notmuch

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

On Wed, Oct 02 2013, Ben Gamari <bgamari.foss@gmail.com> wrote:
> +/* Compacts the given database, optionally saving the original database
> + * in backup_path. Additionally, a callback function can be provided to
> + * give the user feedback on the progress of the (likely long-lived)
> + * compaction process.
> + *
> + * The backup path must point to a directory on the same volume as the
> + * original database. Passing a NULL backup_path will result in the
> + * uncompacted database being deleted after compaction has finished.
> + * Note that the database write lock will be held during the
> + * compaction process to protect data integrity.
> + */
> +notmuch_status_t
> +notmuch_database_compact (const char* path,
> +			  const char* backup_path,
> +			  notmuch_compact_status_cb_t status_cb)
> +{
> +    void *local = talloc_new (NULL);
> +    NotmuchCompactor compactor(status_cb);
> +    char *notmuch_path, *xapian_path, *compact_xapian_path;
> +    char *old_xapian_path = NULL;
> +    notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
> +    notmuch_database_t *notmuch = NULL;
> +    struct stat statbuf;
> +
> +    ret = notmuch_database_open(path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
> +    if (ret) {
> +	goto DONE;
> +    }
> +
> +    if (! (notmuch_path = talloc_asprintf (local, "%s/%s", path, ".notmuch"))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (xapian_path = talloc_asprintf (local, "%s/%s", notmuch_path, "xapian"))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (! (compact_xapian_path = talloc_asprintf (local, "%s.compact", xapian_path))) {
> +	ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	goto DONE;
> +    }
> +
> +    if (backup_path != NULL) {
> +	if (! (old_xapian_path = talloc_asprintf (local, "%s/xapian.old", backup_path))) {
> +	    ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
> +	    goto DONE;
> +	}

Hey, folks.  I'm obviously late for this, but I just got around to
testing the new compact functionality now and I wanted to comment on the
path for the old xapian directory.  It seems to me that

  <notmuch_path>/xapian.old

isn't the right place for it.  I would think that

  <xapian_path>.old

would be a much better place.  I'm not such a fan of dumping internal
notmuch stuff into the main mail directory.  Keeping all notmuch stuff
in <notmuch_path> seems more reasonable and polite.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-11-02 18:31 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-02 20:30 [PATCH] notmuch compact support (v4) Ben Gamari
2013-10-02 20:30 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
2013-10-10 15:09   ` Tomi Ollila
2013-10-11 15:15     ` David Bremner
2013-10-20 17:02     ` Tomi Ollila
2013-11-02 18:30   ` Jameson Graef Rollins
2013-10-02 20:30 ` [PATCH 2/3] notmuch-compact: Initial commit of CLI Ben Gamari
2013-10-02 20:30 ` [PATCH 3/3] notmuch-compact: Add man page Ben Gamari
2013-10-10 10:22 ` [PATCH] notmuch compact support (v4) David Bremner
2013-10-10 12:01   ` Ben Gamari
  -- strict thread matches above, loose matches on Subject: below --
2013-08-24  2:55 Ben Gamari
2013-08-24  2:55 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
2013-09-01 15:43   ` Jani Nikula
2013-09-03 14:32     ` Ben Gamari

Code repositories for project(s) associated with this public inbox

	https://yhetil.org/notmuch.git/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).