* (no subject)
@ 2013-08-24 2:55 Ben Gamari
2013-08-24 2:55 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ben Gamari @ 2013-08-24 2:55 UTC (permalink / raw)
To: notmuch
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [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 2013-08-24 2:55 ` [PATCH 2/3] notmuch-compact: Initial commit Ben Gamari 2013-08-24 2:55 ` [PATCH 3/3] notmuch-compact: Add man page Ben Gamari 2 siblings, 1 reply; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
* [PATCH 2/3] notmuch-compact: Initial commit 2013-08-24 2:55 Ben Gamari 2013-08-24 2:55 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari @ 2013-08-24 2:55 ` Ben Gamari 2013-08-24 2:55 ` [PATCH 3/3] notmuch-compact: Add man page Ben Gamari 2 siblings, 0 replies; 6+ messages in thread From: Ben Gamari @ 2013-08-24 2:55 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 | 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, ¬much)) + 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] notmuch-compact: Add man page 2013-08-24 2:55 Ben Gamari 2013-08-24 2:55 ` [PATCH 1/3] database: Add notmuch_database_compact_close Ben Gamari 2013-08-24 2:55 ` [PATCH 2/3] notmuch-compact: Initial commit Ben Gamari @ 2013-08-24 2:55 ` Ben Gamari 2 siblings, 0 replies; 6+ messages in thread From: Ben Gamari @ 2013-08-24 2:55 UTC (permalink / raw) To: notmuch 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-03 14:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 2013-08-24 2:55 ` [PATCH 2/3] notmuch-compact: Initial commit Ben Gamari 2013-08-24 2:55 ` [PATCH 3/3] notmuch-compact: Add man page 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).