compactor adjustments v2

compactor adjustments v2

[Tomi Ollila]

This is v2 of id:1384192538-15291-1-git-send-email-tomi.ollila@iki.fi 

Changes include: 
  tidy formatting (i.e. "uncrustifying") on related code
  catching Xapian::Error (always)

The tests done in  id:m2fvr1tpkf.fsf@guru.guru-group.fi  were redone
with one extra (*) -- results same. Automatic tests pass.

(*)

$ mkdir xapian.compact
$ touch xapian.compact/file
$ chmod 000 xapian.compact
$ notmuch compact 
Compacting database...
compacting table postlist
Error while compacting: Couldn't open base /path/to/.notmuch/xapian.compact/postlist.baseA to write: Permission denied
Compaction failed: A Xapian exception occurred
zsh: exit 1     ./notmuch compact

[PATCH v2 1/5] compact: tidy formatting

[Tomi Ollila]

Notmuch compact code whitespace changes to match devel/STYLE.
---
 lib/database.cc   | 62 ++++++++++++++++++++++++++++---------------------------
 notmuch-compact.c |  4 ++--
 2 files changed, 34 insertions(+), 32 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index a021bf1..3c008d6 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -805,17 +805,19 @@ notmuch_database_close (notmuch_database_t *notmuch)
 }
 
 #if HAVE_XAPIAN_COMPACT
-static int unlink_cb (const char *path,
-		      unused (const struct stat *sb),
-		      unused (int type),
-		      unused (struct FTW *ftw))
+static int
+unlink_cb (const char *path,
+	   unused (const struct stat *sb),
+	   unused (int type),
+	   unused (struct FTW *ftw))
 {
-    return remove(path);
+    return remove (path);
 }
 
-static int rmtree (const char *path)
+static int
+rmtree (const char *path)
 {
-    return nftw(path, unlink_cb, 64, FTW_DEPTH | FTW_PHYS);
+    return nftw (path, unlink_cb, 64, FTW_DEPTH | FTW_PHYS);
 }
 
 class NotmuchCompactor : public Xapian::Compactor
@@ -825,17 +827,17 @@ class NotmuchCompactor : public Xapian::Compactor
 
 public:
     NotmuchCompactor(notmuch_compact_status_cb_t cb, void *closure) :
-	status_cb(cb), status_closure(closure) { }
+	status_cb (cb), status_closure (closure) { }
 
     virtual void
     set_status (const std::string &table, const std::string &status)
     {
-	char* msg;
+	char *msg;
 
 	if (status_cb == NULL)
 	    return;
 
-	if (status.length() == 0)
+	if (status.length () == 0)
 	    msg = talloc_asprintf (NULL, "compacting table %s", table.c_str());
 	else
 	    msg = talloc_asprintf (NULL, "     %s", status.c_str());
@@ -844,8 +846,8 @@ public:
 	    return;
 	}
 
-	status_cb(msg, status_closure);
-	talloc_free(msg);
+	status_cb (msg, status_closure);
+	talloc_free (msg);
     }
 };
 
@@ -861,8 +863,8 @@ public:
  * compaction process to protect data integrity.
  */
 notmuch_status_t
-notmuch_database_compact (const char* path,
-			  const char* backup_path,
+notmuch_database_compact (const char *path,
+			  const char *backup_path,
 			  notmuch_compact_status_cb_t status_cb,
 			  void *closure)
 {
@@ -876,7 +878,7 @@ notmuch_database_compact (const char* path,
     if (! local)
 	return NOTMUCH_STATUS_OUT_OF_MEMORY;
 
-    ret = notmuch_database_open(path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
+    ret = notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch);
     if (ret) {
 	goto DONE;
     }
@@ -897,25 +899,25 @@ notmuch_database_compact (const char* path,
     }
 
     if (backup_path != NULL) {
-	if (stat(backup_path, &statbuf) != -1) {
+	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));
+		     strerror (errno));
 	    goto DONE;
 	}
     }
 
     try {
-	NotmuchCompactor compactor(status_cb, closure);
+	NotmuchCompactor compactor (status_cb, closure);
 
-	compactor.set_renumber(false);
-	compactor.add_source(xapian_path);
-	compactor.set_destdir(compact_xapian_path);
-	compactor.compact();
+	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;
@@ -923,33 +925,33 @@ notmuch_database_compact (const char* path,
     }
 
     if (backup_path) {
-	if (rename(xapian_path, 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);
+	rmtree (xapian_path);
     }
 
-    if (rename(compact_xapian_path, xapian_path)) {
+    if (rename (compact_xapian_path, xapian_path)) {
 	fprintf (stderr, "Error moving compacted database\n");
 	ret = NOTMUCH_STATUS_FILE_ERROR;
 	goto DONE;
     }
 
-DONE:
+  DONE:
     if (notmuch)
 	notmuch_database_destroy (notmuch);
 
-    talloc_free(local);
+    talloc_free (local);
 
     return ret;
 }
 #else
 notmuch_status_t
-notmuch_database_compact (unused (const char* path),
-			  unused (const char* backup_path),
+notmuch_database_compact (unused (const char *path),
+			  unused (const char *backup_path),
 			  unused (notmuch_compact_status_cb_t status_cb),
 			  unused (void *closure))
 {
@@ -1538,7 +1540,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;
 }
diff --git a/notmuch-compact.c b/notmuch-compact.c
index 8022dfe..8b820c0 100644
--- a/notmuch-compact.c
+++ b/notmuch-compact.c
@@ -23,7 +23,7 @@
 static void
 status_update_cb (const char *msg, unused (void *closure))
 {
-    printf("%s\n", msg);
+    printf ("%s\n", msg);
 }
 
 int
@@ -49,7 +49,7 @@ notmuch_compact_command (notmuch_config_t *config, int argc, char *argv[])
     ret = notmuch_database_compact (path, backup_path,
 				    quiet ? NULL : status_update_cb, NULL);
     if (ret) {
-	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string(ret));
+	fprintf (stderr, "Compaction failed: %s\n", notmuch_status_to_string (ret));
 	return 1;
     }
 
-- 
1.8.3.1

[PATCH v2 2/5] compact: catch Xapian::Error consistently

[Tomi Ollila]

catch Xapian::Error in compact code in lib/database.cc to be consistent
with other code in addition to not making software crash on uncaught
other Xapian error.
---
 lib/database.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3c008d6..3530cb6 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -918,8 +918,8 @@ notmuch_database_compact (const char *path,
 	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());
+    } catch (const Xapian::Error &error) {
+	fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str());
 	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
 	goto DONE;
     }
-- 
1.8.3.1

[PATCH v2 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.
---
 lib/database.cc | 42 +++++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 17 deletions(-)

diff --git a/lib/database.cc b/lib/database.cc
index 3530cb6..ee09c5e 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,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 {
@@ -924,14 +933,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 +945,9 @@ notmuch_database_compact (const char *path,
 	goto DONE;
     }
 
+    if (! keep_backup)
+	rmtree (backup_path);
+
   DONE:
     if (notmuch)
 	notmuch_database_destroy (notmuch);
-- 
1.8.3.1

[PATCH v2 4/5] compact: 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 ee09c5e..4b5ac64 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -920,6 +920,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 v2 5/5] compact: 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. In case such
failure happens, provide better information how to resolve it.

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

diff --git a/lib/database.cc b/lib/database.cc
index 4b5ac64..a6daac6 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -939,19 +939,50 @@ 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: [PATCH v2 4/5] compact: unconditionally attempt to remove old wip database compact directory

[Jani Nikula]

On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> 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 ee09c5e..4b5ac64 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -920,6 +920,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);

I thought we avoid using // comments. Otherwise LGTM.

> +
>      try {
>  	NotmuchCompactor compactor (status_cb, closure);
>  
> -- 
> 1.8.3.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

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

[Jani Nikula]

On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> 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.
> ---
>  lib/database.cc | 42 +++++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3530cb6..ee09c5e 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,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;

*cough* I thought you were the style police around here *cough*

> +
> +    if (stat (backup_path, &statbuf) != -1) {
> +	fprintf (stderr, "Backup path already exists: %s\n", backup_path);

The user will be confused if he specifically didn't add --backup and
happens to get this. But maybe it's a corner case. *shrug*.

> +	ret = NOTMUCH_STATUS_FILE_ERROR;
> +	goto DONE;
> +    }
> +    if (errno != ENOENT) {
> +	fprintf (stderr, "Unknown error while stat()ing backup path: %s\n",
> +		 strerror (errno));

ret = ?

> +	goto DONE;
>      }
>  
>      try {
> @@ -924,14 +933,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 +945,9 @@ notmuch_database_compact (const char *path,
>  	goto DONE;
>      }
>  
> +    if (! keep_backup)
> +	rmtree (backup_path);
> +
>    DONE:
>      if (notmuch)
>  	notmuch_database_destroy (notmuch);
> -- 
> 1.8.3.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures

[Jani Nikula]

On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> After database has been compacted, there are steps to put the new
> database into place -- and these steps may fail. In case such
> failure happens, provide better information how to resolve it.

I disagree with having a library spew all this information out. For each
case, I think it should be sufficient to just say what happened
(e.g. "rename a -> b failed" + strerror). I don't think a library's
error messages should be a tutorial on how to fix things.

We may need to amend notmuch compact man page though.

BR,
Jani.



>
> Thanks to Ben Gamari for most of the information content.
> ---
>  lib/database.cc | 39 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 35 insertions(+), 4 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 4b5ac64..a6daac6 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -939,19 +939,50 @@ 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
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 2/5] compact: catch Xapian::Error consistently

[Jani Nikula]

The first two patches LGTM.

On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> catch Xapian::Error in compact code in lib/database.cc to be consistent
> with other code in addition to not making software crash on uncaught
> other Xapian error.
> ---
>  lib/database.cc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lib/database.cc b/lib/database.cc
> index 3c008d6..3530cb6 100644
> --- a/lib/database.cc
> +++ b/lib/database.cc
> @@ -918,8 +918,8 @@ notmuch_database_compact (const char *path,
>  	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());
> +    } catch (const Xapian::Error &error) {
> +	fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str());
>  	ret = NOTMUCH_STATUS_XAPIAN_EXCEPTION;
>  	goto DONE;
>      }
> -- 
> 1.8.3.1
>
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures

[David Bremner]

Jani Nikula <jani@nikula.org> writes:

> On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>> After database has been compacted, there are steps to put the new
>> database into place -- and these steps may fail. In case such
>> failure happens, provide better information how to resolve it.
>
> I disagree with having a library spew all this information out. For each
> case, I think it should be sufficient to just say what happened
> (e.g. "rename a -> b failed" + strerror). I don't think a library's
> error messages should be a tutorial on how to fix things.
>

I can live with whatever the concensus level of verbosity, but not the
fprintf's; as I mentioned, for once we have a log hook.

That might also somewhat comfort Jani, since the library client has the
option of ignoring the spewing.

d

Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures

[Tomi Ollila]

On Thu, Nov 14 2013, David Bremner <david@tethera.net> wrote:

> Jani Nikula <jani@nikula.org> writes:
>
>> On Wed, 13 Nov 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote:
>>> After database has been compacted, there are steps to put the new
>>> database into place -- and these steps may fail. In case such
>>> failure happens, provide better information how to resolve it.
>>
>> I disagree with having a library spew all this information out. For each
>> case, I think it should be sufficient to just say what happened
>> (e.g. "rename a -> b failed" + strerror). I don't think a library's
>> error messages should be a tutorial on how to fix things.
>>
>
> I can live with whatever the concensus level of verbosity, but not the
> fprintf's; as I mentioned, for once we have a log hook.

Jani is right there that *library* should not dump all that info
to ... stderr. If so much info is printed, the client code should
do that -- but doing that cleanly is not a trivial job. The idea of
amending the namual page to inform user is a good one.

The log hook in it's current form is problematic as it doesn't provide
way to distinguish progress reporting from error reporting. Currently
lib/database.cc writes error messages with fprintf(stderr, ...) everywhere.

I suggest that this problem is fixed in one big sweep during 0.18
development -- the suggestion Jani pastebin'd a few days ago is
a good one and I'm willing to take part of that development...
And now take this approach of fprintf()ing (basically I would
also ask developers using the library wait for 0.18 before starting
to use the compact functionality (if ever), as the we have yet
another soname bump with changing interface coming...


Just that the log interface needs to be in format

void (*log_cb)(void * closure, int level, const char * message)

And we need to decide between the (non-NIH) syslog levels vs.
our own levels...

This way we can distinguish debug (DEBUG), progress (NOTICE, INFO) 
and error (WARN, EER, CRIT, ALERT, EMERG) conditions.


For someone who wished logf(closure, level, fmt, ...) we can provide
helper functions

_log_printf(log_cb, closure, level, fmt, ...) and
_log_vprintf(log_cb, closure, level, fmt, ap)


>
> That might also somewhat comfort Jani, since the library client has the
> option of ignoring the spewing.

String maybe!

>
> d

Tomi

Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures

[David Bremner]

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

> The log hook in it's current form is problematic as it doesn't provide
> way to distinguish progress reporting from error reporting.

Is this _more_ problematic than more output to stderr?

>  Currently
> lib/database.cc writes error messages with fprintf(stderr, ...) everywhere.

Sure. But I'm trying to understand why a partial fix isn't better than
nothing.  Is the argument just that the effort is wasted, or that the
result is somehow less satisfactory than the status quo.

> I suggest that this problem is fixed in one big sweep during 0.18
> development -- the suggestion Jani pastebin'd a few days ago is
> a good one and I'm willing to take part of that development...
> And now take this approach of fprintf()ing (basically I would
> also ask developers using the library wait for 0.18 before starting
> to use the compact functionality (if ever), as the we have yet
> another soname bump with changing interface coming...

I guess we can mark this interface as unstable for the moment?
"Asking developers not to use it" sounds pretty bad.

d

Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures

[Tomi Ollila]

On Sun, Nov 17 2013, David Bremner <david@tethera.net> wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> The log hook in it's current form is problematic as it doesn't provide
>> way to distinguish progress reporting from error reporting.
>
> Is this _more_ problematic than more output to stderr?
>
>>  Currently
>> lib/database.cc writes error messages with fprintf(stderr, ...) everywhere.
>
> Sure. But I'm trying to understand why a partial fix isn't better than
> nothing.  Is the argument just that the effort is wasted, or that the
> result is somehow less satisfactory than the status quo.

The partial fix (using current log hook) would mean we either write all
messages to stdout or to stderr.

To the end user this would mean inconsistent behaviour in compact compared
to other commands (like 'notmuch new' which prints progress to stdout and
errors to stderr).

I personally think doing things this way for 0.17 is tolerable
(considering current schedule and risks involved) so that user experience
is stable.

>> I suggest that this problem is fixed in one big sweep during 0.18
>> development -- the suggestion Jani pastebin'd a few days ago is
>> a good one and I'm willing to take part of that development...
>> And now take this approach of fprintf()ing (basically I would
>> also ask developers using the library wait for 0.18 before starting
>> to use the compact functionality (if ever), as the we have yet
>> another soname bump with changing interface coming...
>
> I guess we can mark this interface as unstable for the moment?
> "Asking developers not to use it" sounds pretty bad.

I was thinking naming the function notmuch_database_compact_internal ()
as one option (I also though of notmuch_database_compact_unstable () -- but
that sounds so "unstable" (at least outside Debian people ;D)) --
could be one option. Then developers should understand the API is not
fixed there...

Although "Informing developers to prepare for upcoming changes to the API"
(which is goind to happen early on 0.18 development cycle) should suffice.

> d

Tomi

Re: [PATCH v2 2/5] compact: catch Xapian::Error consistently

[David Bremner]

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

> catch Xapian::Error in compact code in lib/database.cc to be consistent
> with other code in addition to not making software crash on uncaught
> other Xapian error.

pushed the first two patches

d

Re: [PATCH v2 5/5] compact: provide user more information on after-compaction failures

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:
> I was thinking naming the function notmuch_database_compact_internal ()
> as one option (I also though of notmuch_database_compact_unstable () -- but
> that sounds so "unstable" (at least outside Debian people ;D)) --
> could be one option. Then developers should understand the API is not
> fixed there...

I think 0.18 will just be an API breaking release in general; compact is
probably the least of people's worries. Yes, it's a bit more irritating
because the API will be short lived, but since we plan to change
notmuch_database_open, pretty much every client of the library will have
to update anyway. Luckily there probably aren't enough such clients to
get a really good lynch mob together.  I'd suggest just mentioning it
NEWS for 0.17 that the library interface for compact is expected to
change in 0.18.

d