compactor adjustments

compactor adjustments

[Tomi Ollila]

Hi

I think these changes would be good to have in use before notmuch compact
is in wider usage.

All tests pass. I plan to test all of these code paths manually tomorrow.
If anyone comes up with good plan how to add automatic tests I'll add
those too (I also think myself, haven't got any good ones yet).

[PATCH 1/3] lib: make compact to keep backup until new database in place

[Tomi Ollila]

It is less error prone and window of failure opportunity is smaller
if the old database is always renamed (instead of sometimes rmtree'd)
before new database is put into its place.
Finally rmtree() old database in case old database backup is not kept.
---
 lib/database.cc | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index a021bf1..6b656e9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -871,6 +871,7 @@ notmuch_database_compact (const char* path,
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
     notmuch_database_t *notmuch = NULL;
     struct stat statbuf;
+    bool keep_backup;
 
     local = talloc_new (NULL);
     if (! local)
@@ -896,17 +897,25 @@ notmuch_database_compact (const char* path,
 	goto DONE;
     }
 
-    if (backup_path != NULL) {
-	if (stat(backup_path, &statbuf) != -1) {
-	    fprintf (stderr, "Backup path already exists: %s\n", backup_path);
-	    ret = NOTMUCH_STATUS_FILE_ERROR;
-	    goto DONE;
-	}
-	if (errno != ENOENT) {
-	    fprintf (stderr, "Unknown error while stat()ing backup path: %s\n",
-		     strerror(errno));
+    if (backup_path == NULL) {
+	if (! (backup_path = talloc_asprintf (local, "%s.old", xapian_path))) {
+	    ret = NOTMUCH_STATUS_OUT_OF_MEMORY;
 	    goto DONE;
 	}
+	keep_backup = FALSE;
+    }
+    else
+	keep_backup = TRUE;
+
+    if (stat (backup_path, &statbuf) != -1) {
+	fprintf (stderr, "Backup path already exists: %s\n", backup_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 {
@@ -922,14 +931,10 @@ notmuch_database_compact (const char* path,
 	goto DONE;
     }
 
-    if (backup_path) {
-	if (rename(xapian_path, backup_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(xapian_path, backup_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)) {
@@ -938,6 +943,9 @@ notmuch_database_compact (const char* path,
 	goto DONE;
     }
 
+    if (! keep_backup)
+	rmtree (backup_path);
+
 DONE:
     if (notmuch)
 	notmuch_database_destroy (notmuch);
@@ -1538,7 +1546,7 @@ _notmuch_database_generate_doc_id (notmuch_database_t *notmuch)
     notmuch->last_doc_id++;
 
     if (notmuch->last_doc_id == 0)
-	INTERNAL_ERROR ("Xapian document IDs are exhausted.\n");	
+	INTERNAL_ERROR ("Xapian document IDs are exhausted.\n");
 
     return notmuch->last_doc_id;
 }
-- 
1.8.3.1

[PATCH 2/3] lib: unconditionally attempt to remove old wip database compact directory

[Tomi Ollila]

In case previous notmuch compact has been interrupted there is old
work-in-progress database compact directory partially filled. Remove
it just before starting to fill the directory with new files.
---
 lib/database.cc | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index 6b656e9..78693b7 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -918,6 +918,11 @@ notmuch_database_compact (const char* path,
 	goto DONE;
     }
 
+    // Unconditionally attempt to remove old work-in-progress database (if any).
+    // This is "protected" by database lock. If this fails due to write errors
+    // (etc), the following code will fail and provide error message.
+    (void) rmtree (compact_xapian_path);
+
     try {
 	NotmuchCompactor compactor(status_cb, closure);
 
-- 
1.8.3.1

[PATCH 3/3] lib: provide user more information on after-compaction failures

[Tomi Ollila]

After database has been compacted, there are steps to put the new
database into place -- and these steps may fail. Provide better
information how to resolve these failure cases.

Thanks to Ben Gamari for most of the information content.
---
 lib/database.cc | 38 ++++++++++++++++++++++++++++++++++----
 1 file changed, 34 insertions(+), 4 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 78693b7..40272dc 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -937,19 +937,49 @@ notmuch_database_compact (const char* path,
     }
 
     if (rename(xapian_path, backup_path)) {
-	fprintf (stderr, "Error moving old database out of the way\n");
+	fprintf (stderr, "Error moving old database out of the way:\n"
+		 "Old database: %s\n"
+		 "Backup database: %s\n"
+		 "Error: %s\n", xapian_path, backup_path, strerror (errno));
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
     if (rename(compact_xapian_path, xapian_path)) {
-	fprintf (stderr, "Error moving compacted database\n");
+	fprintf (stderr, "Error moving compacted database into place: %s\n", strerror (errno));
+	fprintf (stderr, "\n"
+		 "Encountered error while moving the compacted database\n"
+		 "\n"
+		 "    %s\n"
+		 "\n"
+		 "to\n"
+		 "\n"
+		 "    %s\n"
+		 "\n"
+		 "Please identify the reason for this and move the compacted database\n"
+		 "into place manually.\n"
+		 "\n"
+		 "Alternatively you can revert to the uncompacted database with\n"
+		 "\n"
+		 "    mv '%s' '%s'\n"
+		 "\n", compact_xapian_path, xapian_path,
+		 backup_path, xapian_path);
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
-    if (! keep_backup)
-	rmtree (backup_path);
+    if (! keep_backup) {
+	if (rmtree (backup_path)) {
+	    fprintf (stderr, "Error removing backup database: %s\n",
+		     strerror (errno));
+	    fprintf (stderr, "\n"
+		     "Please remove the backup database with\n"
+		     "\n"
+		     "   rm -rf '%s'\n" "\n", backup_path);
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+    }
 
 DONE:
     if (notmuch)
-- 
1.8.3.1

Re: compactor adjustments

[Tomi Ollila]

On Mon, Nov 11 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:

> Hi
>
> I think these changes would be good to have in use before notmuch compact
> is in wider usage.
>
> All tests pass. I plan to test all of these code paths manually tomorrow.
> If anyone comes up with good plan how to add automatic tests I'll add
> those too (I also think myself, haven't got any good ones yet).

I made this change to the code:

diff --git a/lib/database.cc b/lib/database.cc
index 40272dc..ad7002b 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -933,2 +933,2 @@ notmuch_database_compact (const char* path,
-    } catch (Xapian::InvalidArgumentError e) {
-       fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str());
+    } catch (Xapian::Error &error) {
+       fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str());

Now it is consistent with other code and also doesn't crash on other errors

I did the following tests:


$ pwd
/path/to/.notmuch


$ mkdir xapian.old
$ notmuch compact
Compacting database...
Backup path already exists: /path/to/.notmuch/xapian.old
Compaction failed: Something went wrong trying to read or write a file
zsh: exit 1     ./notmuch compact


$ : tried to make compact fail by doing:
$ : mkdir xapian.compact / touch xapian.compact + chmod 000 xapian.compact
$ : notmuch compact worked OK after the above.


$ chmod 555 .
$ notmuch compact
Compacting database...
Error while compacting: /path/to/.notmuch/xapian.compact:
cannot create directory
Compaction failed: A Xapian exception occurred
zsh: exit 1     ./notmuch compact


$ chmod 755 .
$ notmuch compact --backup=/tmp/exdev
Compacting database...
...
...
...
Error moving old database out of the way:
Old database: /path/to/.notmuch/xapian
Backup database: /tmp/exdev
Error: Invalid cross-device link
Compaction failed: Something went wrong trying to read or write a file
zsh: exit 1     ./notmuch compact --backup=/tmp/exdev


$ : here I tried all I could think of (that doesn't include modifying
$ : permissions on the fly (perhaps in lib/database.cc code!), but
$ : could not get  if (rename(compact_xapian_path, xapian_path)) {
$ : fail. i.e. it is possible although highly unlikely (which is good). 


$ chmod 555 xapian
Compacting database...
...
...
...
Error removing backup database: Permission denied

Please remove the backup database with

   rm -rf '/path/to/.notmuch/xapian.old'

Compaction failed: Something went wrong trying to read or write a file
zsh: exit 1     ./notmuch compact


$ chmod 755 xapian
$ notmuch compact --backup=xapian.backup
Compacting database...
...
...
...
The old database has been moved to xapian.backup.
Done.
$ ls xapian.backup
flintlock       postlist.baseB  record.baseB    termlist.baseB
iamchert        postlist.DB     record.DB       termlist.DB
postlist.baseA  record.baseA    termlist.baseA


---

Unless there are other comments I'll make new patch series where
 } catch (Xapian::Error &error) { and submit it for inclusion to 0.17.
we'd better have somewhat decent 'notmuch compact' from day one.


Tomi