(no subject)

[Ben Gamari]

From: Ben Gamari <bgamari.foss@gmail.com>
Subject: [PATCH] notmuch compact support (v3)
In-Reply-To: 

Here is the latest (and hopefully last) iteration of my patchset introducing a
compact command. The set has been rebased on top of master, a manpage has been
added, and error handling has been greatly improved.

Cheers,

- Ben

[PATCH 1/3] database: Add notmuch_database_compact_close

[Ben Gamari]

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

[PATCH 2/3] notmuch-compact: Initial commit

[Ben Gamari]

Introduce the user command exposing the new compaction facility.

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 Makefile.local    |  1 +
 notmuch-client.h  |  6 ++++++
 notmuch-compact.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 notmuch.c         |  2 ++
 4 files changed, 52 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 da332f3..5b8a076 100644
--- a/notmuch-client.h
+++ b/notmuch-client.h
@@ -203,6 +203,12 @@ 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[]);
+
+int
+notmuch_config_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..72b5c1e
--- /dev/null
+++ b/notmuch-compact.c
@@ -0,0 +1,43 @@
+/* notmuch - Not much of an email program, (just index and search)
+ *
+ * Copyright © 2013 Ben Gmaari
+ *
+ * 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"
+
+int
+notmuch_compact_command (notmuch_config_t *config, unused (int argc), unused (char *argv[]))
+{
+    notmuch_database_t *notmuch;
+    notmuch_status_t ret;
+
+    if (notmuch_database_open (notmuch_config_get_database_path (config),
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
+	return 1;
+
+    printf ("Compacting database...\n");
+    ret = notmuch_database_compact_close (notmuch);
+    if (ret) {
+	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
+    } else {
+	printf ("Done.\n");
+    }
+    notmuch_database_destroy (notmuch);
+
+    return 0;
+}
diff --git a/notmuch.c b/notmuch.c
index 78d29a8..ab0b1cf 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

[PATCH 3/3] notmuch-compact: Add man page

[Ben Gamari]

Signed-off-by: Ben Gamari <bgamari.foss@gmail.com>
---
 man/Makefile.local         |  1 +
 man/man1/notmuch-compact.1 | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 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..48f76c3
--- /dev/null
+++ b/man/man1/notmuch-compact.1
@@ -0,0 +1,33 @@
+.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 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.
+
+.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

Re: [PATCH 1/3] database: Add notmuch_database_compact_close

[Jani Nikula]

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

Re: [PATCH 1/3] database: Add notmuch_database_compact_close

[Ben Gamari]

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

[PATCH 1/3] database: Add notmuch_database_compact_close

[Ben Gamari]

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

Re: [PATCH 1/3] database: Add notmuch_database_compact_close

[Tomi Ollila]

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

Re: [PATCH 1/3] database: Add notmuch_database_compact_close

[David Bremner]

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

Re: [PATCH 1/3] database: Add notmuch_database_compact_close

[Tomi Ollila]

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

Re: [PATCH 1/3] database: Add notmuch_database_compact_close

[Jameson Graef Rollins]

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