v4 of DB_RETRY_LOCK patches

v4 of DB_RETRY_LOCK patches

[David Bremner]

This obsoletes

   id:1465043356-23420-2-git-send-email-david@tethera.net

I investigated adding a timeout (including talking a bit to Xapian
upstream about it), and thought about some runtime configuration
options, but for now I settled on a configure. Depending what
experiences people report, we would make the default disabled for the
next release.

[PATCH 1/3] Use the Xapian::DB_RETRY_LOCK flag when available

[David Bremner]

From: Istvan Marko <notmuch@kismala.com>

Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
275). Detect it in configure and optionally use it. With this flag
commands that need the write lock will wait for their turn instead of
aborting when it's not immediately available.

Amended by db: allow disabling in configure
---
 configure       | 35 +++++++++++++++++++++++++++++++++++
 lib/database.cc |  8 +++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 49fa5b9..ae0a027 100755
--- a/configure
+++ b/configure
@@ -72,6 +72,7 @@ WITH_EMACS=1
 WITH_BASH=1
 WITH_RUBY=1
 WITH_ZSH=1
+WITH_RETRY_LOCK=1
 
 usage ()
 {
@@ -140,6 +141,7 @@ Some features can be disabled (--with-feature=no is equivalent to
 	--without-emacs			Do not install lisp file
 	--without-ruby			Do not install ruby bindings
 	--without-zsh-completion	Do not install zsh completions files
+	--without-retry-lock		Do not use blocking xapian opens, even if available
 
 Additional options are accepted for compatibility with other
 configure-script calling conventions, but don't do anything yet:
@@ -211,6 +213,14 @@ for option; do
 	fi
     elif [ "${option}" = '--without-ruby' ] ; then
 	WITH_RUBY=0
+    elif [ "${option%%=*}" = '--with-retry-lock' ]; then
+	if [ "${option#*=}" = 'no' ]; then
+	    WITH_RETRY_LOCK=0
+	else
+	    WITH_RETRY_LOCK=1
+	fi
+    elif [ "${option}" = '--without-retry-lock' ] ; then
+	WITH_RETRY_LOCK=0
     elif [ "${option%%=*}" = '--with-zsh-completion' ]; then
 	if [ "${option#*=}" = 'no' ]; then
 	    WITH_ZSH=0
@@ -392,6 +402,24 @@ EOF
     rm -f _field_processor.o _field_processor.cc
 
     default_xapian_backend=""
+    # DB_RETRY_LOCK is only supported on Xapian > 1.3.2
+    have_xapian_db_retry_lock=0
+    if [ $WITH_RETRY_LOCK = "1" ]; then
+	printf "Checking for Xapian lock retry support... "
+	cat>_retry.cc<<EOF
+#include <xapian.h>
+int flag = Xapian::DB_RETRY_LOCK;
+EOF
+	if ${CXX} ${CXXFLAGS_for_sh} ${xapian_cxxflags} -c _retry.cc -o _retry.o > /dev/null 2>&1
+	then
+	    have_xapian_db_retry_lock=1
+	    printf "Yes.\n"
+	else
+	    printf "No. (optional)\n"
+	fi
+	rm -f _retry.o _retry.cc
+    fi
+
     printf "Testing default Xapian backend... "
     cat >_default_backend.cc <<EOF
 #include <xapian.h>
@@ -1022,6 +1050,9 @@ HAVE_XAPIAN_COMPACT = ${have_xapian_compact}
 # Whether the Xapian version in use supports field processors
 HAVE_XAPIAN_FIELD_PROCESSOR = ${have_xapian_field_processor}
 
+# Whether the Xapian version in use supports DB_RETRY_LOCK
+HAVE_XAPIAN_DB_RETRY_LOCK = ${have_xapian_db_retry_lock}
+
 # 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)
@@ -1097,6 +1128,7 @@ COMMON_CONFIGURE_CFLAGS = \\
 	-DSTD_ASCTIME=\$(STD_ASCTIME)				\\
 	-DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT)		\\
 	-DHAVE_XAPIAN_FIELD_PROCESSOR=\$(HAVE_XAPIAN_FIELD_PROCESSOR) \\
+	-DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\
 	-DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER)
 
 CONFIGURE_CFLAGS = \$(COMMON_CONFIGURE_CFLAGS)
@@ -1117,6 +1149,9 @@ NOTMUCH_HAVE_XAPIAN_COMPACT=${have_xapian_compact}
 # Whether the Xapian version in use supports field processors
 NOTMUCH_HAVE_XAPIAN_FIELD_PROCESSOR=${have_xapian_field_processor}
 
+# Whether the Xapian version in use supports lock retry
+NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${have_xapian_db_retry_lock}
+
 # Which backend will Xapian use by default?
 NOTMUCH_DEFAULT_XAPIAN_BACKEND=${default_xapian_backend}
 
diff --git a/lib/database.cc b/lib/database.cc
index afafe88..66ee267 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -49,6 +49,12 @@ typedef struct {
 #define STRINGIFY(s) _SUB_STRINGIFY(s)
 #define _SUB_STRINGIFY(s) #s
 
+#if HAVE_XAPIAN_DB_RETRY_LOCK
+#define DB_ACTION (Xapian::DB_CREATE_OR_OPEN | Xapian::DB_RETRY_LOCK)
+#else
+#define DB_ACTION Xapian::DB_CREATE_OR_OPEN
+#endif
+
 /* Here's the current schema for our database (for NOTMUCH_DATABASE_VERSION):
  *
  * We currently have three different types of documents (mail, ghost,
@@ -939,7 +945,7 @@ notmuch_database_open_verbose (const char *path,
 
 	if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) {
 	    notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path,
-							       Xapian::DB_CREATE_OR_OPEN);
+							       DB_ACTION);
 	} else {
 	    notmuch->xapian_db = new Xapian::Database (xapian_path);
 	}
-- 
2.8.1

Re: [PATCH 1/3] Use the Xapian::DB_RETRY_LOCK flag when available

[David Bremner]

David Bremner <david@tethera.net> writes:

> From: Istvan Marko <notmuch@kismala.com>
>
> Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug
> 275). Detect it in configure and optionally use it. With this flag
> commands that need the write lock will wait for their turn instead of
> aborting when it's not immediately available.

series pushed

d

[PATCH 2/3] test: initial tests for locking retry

[David Bremner]

Currently there's not much to test, so we simulate contention, and check
that the modifications to the database are serialized.
---
 test/T620-lock.sh | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100755 test/T620-lock.sh

diff --git a/test/T620-lock.sh b/test/T620-lock.sh
new file mode 100755
index 0000000..f46475e
--- /dev/null
+++ b/test/T620-lock.sh
@@ -0,0 +1,75 @@
+#!/usr/bin/env bash
+test_description="locking"
+. ./test-lib.sh || exit 1
+
+if [ "${NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK}" = "0" ]; then
+    test_subtest_missing_external_prereq_["lock retry support"]=t
+fi
+
+add_email_corpus
+
+test_begin_subtest "blocking open"
+test_C ${MAIL_DIR} <<'EOF'
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/wait.h>
+#include <notmuch-test.h>
+
+void
+taggit (notmuch_database_t *db, const char *tag)
+{
+    notmuch_message_t *message;
+
+    EXPECT0 (notmuch_database_find_message (db, "4EFC743A.3060609@april.org", &message));
+    if (message == NULL) {
+	fprintf (stderr, "unable to find message");
+	exit (1);
+    }
+
+    EXPECT0 (notmuch_message_add_tag (message, tag));
+    notmuch_message_destroy (message);
+}
+
+int
+main (int argc, char **argv)
+{
+    pid_t child;
+    const char *path = argv[1];
+
+    child = fork ();
+    if (child == -1) {
+	fprintf (stderr, "fork failed\n");
+	exit (1);
+    }
+
+    if (child == 0) {
+	notmuch_database_t *db2;
+
+	sleep (1);
+	EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &db2));
+	taggit (db2, "child");
+	EXPECT0 (notmuch_database_close (db2));
+    } else {
+	notmuch_database_t *db;
+
+	EXPECT0 (notmuch_database_open (path, NOTMUCH_DATABASE_MODE_READ_WRITE, &db));
+	taggit (db, "parent");
+	sleep (2);
+	EXPECT0 (notmuch_database_close (db));
+	wait (NULL);
+    }
+}
+
+EOF
+notmuch search --output=tags id:4EFC743A.3060609@april.org >> OUTPUT
+cat <<'EOF' >EXPECTED
+== stdout ==
+== stderr ==
+child
+inbox
+parent
+unread
+EOF
+test_expect_equal_file EXPECTED OUTPUT
+
+test_done
-- 
2.8.1

[PATCH 3/3] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK

[David Bremner]

This support will be present only if the appropriate version of xapian
is available _and_ the user did not disable the feature when
building. So there really needs to be some way for the user to check.
---
 lib/built-with.c    | 2 ++
 notmuch-config.c    | 3 +++
 test/T030-config.sh | 1 +
 test/T040-setup.sh  | 3 ++-
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/lib/built-with.c b/lib/built-with.c
index 635ed3b..2f1f0b5 100644
--- a/lib/built-with.c
+++ b/lib/built-with.c
@@ -28,6 +28,8 @@ notmuch_built_with (const char *name)
 	return HAVE_XAPIAN_COMPACT;
     } else if (STRNCMP_LITERAL (name, "field_processor") == 0) {
 	return HAVE_XAPIAN_FIELD_PROCESSOR;
+    } else if (STRNCMP_LITERAL (name, "retry_lock") == 0) {
+	return HAVE_XAPIAN_DB_RETRY_LOCK;
     } else {
 	return FALSE;
     }
diff --git a/notmuch-config.c b/notmuch-config.c
index de9a8a4..e5d42a0 100644
--- a/notmuch-config.c
+++ b/notmuch-config.c
@@ -911,6 +911,9 @@ _notmuch_config_list_built_with ()
     printf("%sfield_processor=%s\n",
 	   BUILT_WITH_PREFIX,
 	   notmuch_built_with ("field_processor") ? "true" : "false");
+    printf("%sretry_lock=%s\n",
+	   BUILT_WITH_PREFIX,
+	   notmuch_built_with ("retry_lock") ? "true" : "false");
 }
 
 static int
diff --git a/test/T030-config.sh b/test/T030-config.sh
index b8d5a86..0915abd 100755
--- a/test/T030-config.sh
+++ b/test/T030-config.sh
@@ -59,6 +59,7 @@ foo.string=this is another string value
 foo.list=this;is another;list value;
 built_with.compact=something
 built_with.field_processor=something
+built_with.retry_lock=something
 EOF
 test_expect_equal_file EXPECTED OUTPUT
 
diff --git a/test/T040-setup.sh b/test/T040-setup.sh
index be2f0db..021f2d0 100755
--- a/test/T040-setup.sh
+++ b/test/T040-setup.sh
@@ -31,6 +31,7 @@ search.exclude_tags=baz;
 maildir.synchronize_flags=true
 crypto.gpg_path=gpg
 built_with.compact=something
-built_with.field_processor=something"
+built_with.field_processor=something
+built_with.retry_lock=something"
 
 test_done
-- 
2.8.1

Re: v4 of DB_RETRY_LOCK patches

[Istvan Marko]

Thanks for getting this into shape, LGTM.

FWIW, I have been running with DB_RETRY_LOCK for the past couple of
months on a fairly large mail spool (500K emails), a high incoming
volume and many tagging rules. So far it's been working great. No
deadlocks, any delays waiting for locks are barely noticeable and it's
so nice not having to deal with those annoying "already locked"
failures.

-- 
	Istvan

Re: v4 of DB_RETRY_LOCK patches

[Tomi Ollila]

On Sun, Jun 26 2016, David Bremner <david@tethera.net> wrote:

> This obsoletes
>
>    id:1465043356-23420-2-git-send-email-david@tethera.net
>
> I investigated adding a timeout (including talking a bit to Xapian
> upstream about it), and thought about some runtime configuration
> options, but for now I settled on a configure. Depending what
> experiences people report, we would make the default disabled for the
> next release.

Looks good, tests pass (*)

Tomi

I.e.

 missing prerequisites: lock retry support
  SKIP   all tests in T620-lock