[PATCH v3 3/5] compact: preserve backup database until compacted database is in place

[PATCH v3 3/5] compact: preserve backup database until compacted database is in place

[Tomi Ollila]

It is less error prone and window of failure opportunity is smaller
if the old (backup) database is always renamed (instead of sometimes
rmtree'd) before new (compacted) database is put into its place.
Finally rmtree() old database in case old database backup is not kept.
---

The patches 3-5 in this series are updated based on Jani's comments.

I've kept the fprintf(stderr,...)'s due to reasons explained in

id:m28uwquccd.fsf@guru.guru-group.fi

Diff v2-v3 for first (3/5) patch. Second (4/5) has just // -> /* .. */ change.

|--- a/lib/database.cc
|+++ b/lib/database.cc
|@@ -906,17 +906,19 @@ notmuch_database_compact (const char *path,
|        }
|        keep_backup = FALSE;
|     }
|-    else
|+    else {
|        keep_backup = TRUE;
|+    }
|
|     if (stat (backup_path, &statbuf) != -1) {
|-       fprintf (stderr, "Backup path already exists: %s\n", backup_path);
|+       fprintf (stderr, "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",
|+       fprintf (stderr, "Unknown error while stat()ing path: %s\n",
|                 strerror (errno));
|+       ret = NOTMUCH_STATUS_FILE_ERROR;
|        goto DONE;
|     }




 lib/database.cc | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3530cb6..d79cc30 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -873,6 +873,7 @@ notmuch_database_compact (const char *path,
     notmuch_status_t ret = NOTMUCH_STATUS_SUCCESS;
     notmuch_database_t *notmuch = NULL;
     struct stat statbuf;
+    notmuch_bool_t keep_backup;
 
     local = talloc_new (NULL);
     if (! local)
@@ -898,17 +899,27 @@ 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, "Path already exists: %s\n", backup_path);
+	ret = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
+    }
+    if (errno != ENOENT) {
+	fprintf (stderr, "Unknown error while stat()ing path: %s\n",
+		 strerror (errno));
+	ret = NOTMUCH_STATUS_FILE_ERROR;
+	goto DONE;
     }
 
     try {
@@ -924,14 +935,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)) {
@@ -940,6 +947,9 @@ notmuch_database_compact (const char *path,
 	goto DONE;
     }
 
+    if (! keep_backup)
+	rmtree (backup_path);
+
   DONE:
     if (notmuch)
 	notmuch_database_destroy (notmuch);
-- 
1.8.0

[PATCH v3 4/5] compact: unconditionally 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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/database.cc b/lib/database.cc
index d79cc30..d09ad99 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -922,6 +922,12 @@ 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.0

[PATCH 5/5] compact: improve error messages on failures after compaction

[Tomi Ollila]

The error messages written during the steps replacing old
database with new now includes relevant paths and strerror.
---
 lib/database.cc | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index d09ad99..f395061 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -942,19 +942,27 @@ 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 %s to %s: %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 %s to %s: %s\n",
+		 compact_xapian_path, xapian_path, strerror (errno));
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
-    if (! keep_backup)
-	rmtree (backup_path);
+    if (! keep_backup) {
+	if (rmtree (backup_path)) {
+	    fprintf (stderr, "Error removing old database %s: %s\n",
+		     backup_path, strerror (errno));
+	    ret = NOTMUCH_STATUS_FILE_ERROR;
+	    goto DONE;
+	}
+    }
 
   DONE:
     if (notmuch)
-- 
1.8.0

Re: [PATCH v3 3/5] compact: preserve backup database until compacted database is in place

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> It is less error prone and window of failure opportunity is smaller
> if the old (backup) database is always renamed (instead of sometimes
> rmtree'd) before new (compacted) database is put into its place.
> Finally rmtree() old database in case old database backup is not kept.

pushed, 

d