* [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available @ 2016-05-03 15:09 Istvan Marko 2016-05-03 16:31 ` Jani Nikula 2016-05-03 19:01 ` [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner 0 siblings, 2 replies; 13+ messages in thread From: Istvan Marko @ 2016-05-03 15:09 UTC (permalink / raw) To: notmuch Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug 275). Detect it in configure and use it if available. With this flag commands that need the write lock will wait for their turn instead of aborting when it's not immediately available. --- configure | 25 ++++++++++++++++++++++++- lib/database.cc | 5 +++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 6231d2b..0c1d5bc 100755 --- a/configure +++ b/configure @@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then esac fi +# DB_RETRY_LOCK is only supported on Xapian > 1.3.2 +have_xapian_db_retry_lock=0 +if [ ${have_xapian} = "1" ]; then + printf "Checking for Xapian lock retry support... " + case "${xapian_version}" in + 0.*|1.[012].*|1.3.[0-2]) + printf "No (only available with Xapian > 1.3.2).\n" ;; + [1-9]*.[0-9]*.[0-9]*) + have_xapian_db_retry_lock=1 + printf "Yes.\n" ;; + *) + printf "Unknown version.\n" ;; + esac +fi + default_xapian_backend="" if [ ${have_xapian} = "1" ]; then printf "Testing default Xapian backend... " @@ -998,6 +1013,9 @@ HAVE_D_TYPE = ${have_d_type} # Whether the Xapian version in use supports compaction HAVE_XAPIAN_COMPACT = ${have_xapian_compact} +# 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) @@ -1072,6 +1090,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ -DSTD_GETPWUID=\$(STD_GETPWUID) \\ -DSTD_ASCTIME=\$(STD_ASCTIME) \\ -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT) \\ + -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\ -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER) CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ @@ -1086,6 +1105,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ -DSTD_GETPWUID=\$(STD_GETPWUID) \\ -DSTD_ASCTIME=\$(STD_ASCTIME) \\ -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT) \\ + -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\ -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER) CONFIGURE_LDFLAGS = \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS) @@ -1097,7 +1117,10 @@ cat > sh.config <<EOF # script of notmuch. # Whether the Xapian version in use supports compaction -NOTMUCH_HAVE_XAPIAN_COMPACT=${have_xapian_compact} +NOTMUCH_HAVE_XAPIAN_ =${have_xapian_compact} + +# Whether the Xapian version in use supports DB_RETRY_LOCK +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 c8c5e26..4b503a2 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -929,8 +929,13 @@ notmuch_database_open_verbose (const char *path, string last_mod; if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) { + #if HAVE_XAPIAN_DB_RETRY_LOCK + notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path, + Xapian::DB_CREATE_OR_OPEN|Xapian::DB_RETRY_LOCK); + #else notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path, Xapian::DB_CREATE_OR_OPEN); + #endif } else { notmuch->xapian_db = new Xapian::Database (xapian_path); } -- 2.4.10 -- Istvan ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available 2016-05-03 15:09 [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available Istvan Marko @ 2016-05-03 16:31 ` Jani Nikula 2016-05-03 19:12 ` [PATCH v2] " Istvan Marko 2016-05-03 19:01 ` [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner 1 sibling, 1 reply; 13+ messages in thread From: Jani Nikula @ 2016-05-03 16:31 UTC (permalink / raw) To: Istvan Marko, notmuch On Tue, 03 May 2016, Istvan Marko <notmuch@kismala.com> wrote: > Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug > 275). Detect it in configure and use it if available. With this flag > commands that need the write lock will wait for their turn instead of > aborting when it's not immediately available. > --- > configure | 25 ++++++++++++++++++++++++- > lib/database.cc | 5 +++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/configure b/configure > index 6231d2b..0c1d5bc 100755 > --- a/configure > +++ b/configure > @@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then > esac > fi > > +# DB_RETRY_LOCK is only supported on Xapian > 1.3.2 > +have_xapian_db_retry_lock=0 > +if [ ${have_xapian} = "1" ]; then > + printf "Checking for Xapian lock retry support... " > + case "${xapian_version}" in > + 0.*|1.[012].*|1.3.[0-2]) > + printf "No (only available with Xapian > 1.3.2).\n" ;; > + [1-9]*.[0-9]*.[0-9]*) > + have_xapian_db_retry_lock=1 > + printf "Yes.\n" ;; > + *) > + printf "Unknown version.\n" ;; > + esac > +fi > + > default_xapian_backend="" > if [ ${have_xapian} = "1" ]; then > printf "Testing default Xapian backend... " > @@ -998,6 +1013,9 @@ HAVE_D_TYPE = ${have_d_type} > # Whether the Xapian version in use supports compaction > HAVE_XAPIAN_COMPACT = ${have_xapian_compact} > > +# 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) > @@ -1072,6 +1090,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ > -DSTD_GETPWUID=\$(STD_GETPWUID) \\ > -DSTD_ASCTIME=\$(STD_ASCTIME) \\ > -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT) \\ > + -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\ > -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER) > > CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ > @@ -1086,6 +1105,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ > -DSTD_GETPWUID=\$(STD_GETPWUID) \\ > -DSTD_ASCTIME=\$(STD_ASCTIME) \\ > -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT) \\ > + -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\ > -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER) > > CONFIGURE_LDFLAGS = \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS) > @@ -1097,7 +1117,10 @@ cat > sh.config <<EOF > # script of notmuch. > > # Whether the Xapian version in use supports compaction > -NOTMUCH_HAVE_XAPIAN_COMPACT=${have_xapian_compact} > +NOTMUCH_HAVE_XAPIAN_ =${have_xapian_compact} Ooops? > + > +# Whether the Xapian version in use supports DB_RETRY_LOCK > +NOTMUCH_HAVE_XAPIAN_DB_RETRY_LOCK=${have_xapian_db_retry_lock} You don't need to update sh.config unless you need this in a shell script, typically in the tests. Which brings us to the question, how can you test this? > > # 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 c8c5e26..4b503a2 100644 > --- a/lib/database.cc > +++ b/lib/database.cc > @@ -929,8 +929,13 @@ notmuch_database_open_verbose (const char *path, > string last_mod; > > if (mode == NOTMUCH_DATABASE_MODE_READ_WRITE) { > + #if HAVE_XAPIAN_DB_RETRY_LOCK > + notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path, > + Xapian::DB_CREATE_OR_OPEN|Xapian::DB_RETRY_LOCK); > + #else > notmuch->xapian_db = new Xapian::WritableDatabase (xapian_path, > Xapian::DB_CREATE_OR_OPEN); > + #endif Please no #ifdefs like this in the middle of the code. A better alternative is to define something like this above the function: #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 and use that in new Xapian::WritableDatabase(). BR, Jani. > } else { > notmuch->xapian_db = new Xapian::Database (xapian_path); > } > -- > 2.4.10 > > > -- > Istvan > _______________________________________________ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] Use the Xapian::DB_RETRY_LOCK flag when available 2016-05-03 16:31 ` Jani Nikula @ 2016-05-03 19:12 ` Istvan Marko 2016-05-05 10:33 ` David Bremner 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner 0 siblings, 2 replies; 13+ messages in thread From: Istvan Marko @ 2016-05-03 19:12 UTC (permalink / raw) To: Jani Nikula, notmuch Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug 275). Detect it in configure and use it if available. With this flag commands that need the write lock will wait for their turn instead of aborting when it's not immediately available. --- Updated based on Jani's feedback. Regarding tests, I couldn't think of a meaningful test for this. We get different behaviors depending on the Xapian version and if we try do some kind of a concurrent locking test we'd really be just testing Xapian's locking code rather than notmuch. configure | 20 ++++++++++++++++++++ lib/database.cc | 8 +++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 6231d2b..8ef96b4 100755 --- a/configure +++ b/configure @@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then esac fi +# DB_RETRY_LOCK is only supported on Xapian > 1.3.2 +have_xapian_db_retry_lock=0 +if [ ${have_xapian} = "1" ]; then + printf "Checking for Xapian lock retry support... " + case "${xapian_version}" in + 0.*|1.[012].*|1.3.[0-2]) + printf "No (only available with Xapian > 1.3.2).\n" ;; + [1-9]*.[0-9]*.[0-9]*) + have_xapian_db_retry_lock=1 + printf "Yes.\n" ;; + *) + printf "Unknown version.\n" ;; + esac +fi + default_xapian_backend="" if [ ${have_xapian} = "1" ]; then printf "Testing default Xapian backend... " @@ -998,6 +1013,9 @@ HAVE_D_TYPE = ${have_d_type} # Whether the Xapian version in use supports compaction HAVE_XAPIAN_COMPACT = ${have_xapian_compact} +# 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) @@ -1072,6 +1090,7 @@ CONFIGURE_CFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ -DSTD_GETPWUID=\$(STD_GETPWUID) \\ -DSTD_ASCTIME=\$(STD_ASCTIME) \\ -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT) \\ + -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\ -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER) CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ @@ -1086,6 +1105,7 @@ CONFIGURE_CXXFLAGS = -DHAVE_GETLINE=\$(HAVE_GETLINE) \$(GMIME_CFLAGS) \\ -DSTD_GETPWUID=\$(STD_GETPWUID) \\ -DSTD_ASCTIME=\$(STD_ASCTIME) \\ -DHAVE_XAPIAN_COMPACT=\$(HAVE_XAPIAN_COMPACT) \\ + -DHAVE_XAPIAN_DB_RETRY_LOCK=\$(HAVE_XAPIAN_DB_RETRY_LOCK) \\ -DUTIL_BYTE_ORDER=\$(UTIL_BYTE_ORDER) CONFIGURE_LDFLAGS = \$(GMIME_LDFLAGS) \$(TALLOC_LDFLAGS) \$(ZLIB_LDFLAGS) \$(XAPIAN_LDFLAGS) diff --git a/lib/database.cc b/lib/database.cc index c8c5e26..c0fcb29 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -48,6 +48,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, @@ -930,7 +936,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.4.10 -- Istvan ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] Use the Xapian::DB_RETRY_LOCK flag when available 2016-05-03 19:12 ` [PATCH v2] " Istvan Marko @ 2016-05-05 10:33 ` David Bremner 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner 1 sibling, 0 replies; 13+ messages in thread From: David Bremner @ 2016-05-05 10:33 UTC (permalink / raw) To: Istvan Marko, Jani Nikula, notmuch Istvan Marko <notmuch@kismala.com> writes: > Regarding tests, I couldn't think of a meaningful test for this. We get > different behaviors depending on the Xapian version and if we try do > some kind of a concurrent locking test we'd really be just testing > Xapian's locking code rather than notmuch. We would be testing the build/configuration part to make sure we are enabling the right flags, but more importantly testing the error handling code, since we have to catch exceptions and pass them back through the C library interface as error codes. One thing that would make this easier to test is adding some finer grain errors than "NOTMUCH_STATUS_XAPIAN_EXCEPTION" for notmuch_database_open_verbose to return, to distinguish between the case of immediate failure and timeout. Although it's not clear to me that Xapian actually provides a timeout when DB_RETRY_LOCK is used. ^ permalink raw reply [flat|nested] 13+ messages in thread
* v3 of DB_RETRY_LOCK 2016-05-03 19:12 ` [PATCH v2] " Istvan Marko 2016-05-05 10:33 ` David Bremner @ 2016-06-04 12:29 ` David Bremner 2016-06-04 12:29 ` [PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner ` (3 more replies) 1 sibling, 4 replies; 13+ messages in thread From: David Bremner @ 2016-06-04 12:29 UTC (permalink / raw) To: Istvan Marko, Jani Nikula, notmuch [PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available This is the original patch, rebased on master. I took the liberty of converting the configure test to a test compile. [PATCH 2/4] test: factor out some boilerplate from C tests [PATCH 3/4] test: initial tests for locking retry Add a simple test of locking retry. [PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK Make it visible to the user whether their notmuch supports locking retry (blocking open). The remaining question for me is if we are happy with defaulting to blocking (w/o timeout) on open. I'm not sure how you could deadlock without writing C code, but it's pretty easy with fork. As I discovered writing my test case. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner @ 2016-06-04 12:29 ` David Bremner 2016-06-04 12:29 ` [PATCH 2/4] test: factor out some boilerplate from C tests David Bremner ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: David Bremner @ 2016-06-04 12:29 UTC (permalink / raw) To: Istvan Marko, Jani Nikula, notmuch From: Istvan Marko <notmuch@kismala.com> Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug 275). Detect it in configure and use it if available. With this flag commands that need the write lock will wait for their turn instead of aborting when it's not immediately available. --- configure | 20 ++++++++++++++++++++ lib/database.cc | 8 +++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/configure b/configure index ce1d698..e9de9c6 100755 --- a/configure +++ b/configure @@ -392,6 +392,22 @@ 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 + 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 + printf "Testing default Xapian backend... " cat >_default_backend.cc <<EOF #include <xapian.h> @@ -1022,6 +1038,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 +1116,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) diff --git a/lib/database.cc b/lib/database.cc index 9630000..e4f3ab5 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, @@ -931,7 +937,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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] test: factor out some boilerplate from C tests 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner 2016-06-04 12:29 ` [PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner @ 2016-06-04 12:29 ` David Bremner 2016-06-04 12:37 ` [PATCH] fixup! " David Bremner 2016-06-14 0:57 ` [PATCH 2/4] " David Bremner 2016-06-04 12:29 ` [PATCH 3/4] test: initial tests for locking retry David Bremner 2016-06-04 12:29 ` [PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK David Bremner 3 siblings, 2 replies; 13+ messages in thread From: David Bremner @ 2016-06-04 12:29 UTC (permalink / raw) To: Istvan Marko, Jani Nikula, notmuch The trick of having a common header file doesn't work to share between test scripts, so make an include file in the test directory. --- test/T590-libconfig.sh | 39 ++++++++++++++------------------------- test/notmuch-test.h | 15 +++++++++++++++ test/test-lib.sh | 2 +- 3 files changed, 30 insertions(+), 26 deletions(-) create mode 100644 test/notmuch-test.h diff --git a/test/T590-libconfig.sh b/test/T590-libconfig.sh index 9c1e566..e8c078d 100755 --- a/test/T590-libconfig.sh +++ b/test/T590-libconfig.sh @@ -6,20 +6,9 @@ test_description="library config API" add_email_corpus cat <<EOF > c_head -#include <stdio.h> #include <string.h> #include <stdlib.h> -#include <notmuch.h> - -void run(int line, notmuch_status_t ret) -{ - if (ret) { - fprintf (stderr, "line %d: %s\n", line, ret); - exit (1); - } -} - -#define RUN(v) run(__LINE__, v); +#include <notmuch-test.h> int main (int argc, char** argv) { @@ -27,23 +16,23 @@ int main (int argc, char** argv) char *val; notmuch_status_t stat; - RUN(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db)); + EXPECT0(notmuch_database_open (argv[1], NOTMUCH_DATABASE_MODE_READ_WRITE, &db)); EOF cat <<EOF > c_tail - RUN(notmuch_database_destroy(db)); + EXPECT0(notmuch_database_destroy(db)); } EOF test_begin_subtest "notmuch_database_{set,get}_config" cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} { - RUN(notmuch_database_set_config (db, "testkey1", "testvalue1")); - RUN(notmuch_database_set_config (db, "testkey2", "testvalue2")); - RUN(notmuch_database_get_config (db, "testkey1", &val)); + EXPECT0(notmuch_database_set_config (db, "testkey1", "testvalue1")); + EXPECT0(notmuch_database_set_config (db, "testkey2", "testvalue2")); + EXPECT0(notmuch_database_get_config (db, "testkey1", &val)); printf("testkey1 = %s\n", val); - RUN(notmuch_database_get_config (db, "testkey2", &val)); + EXPECT0(notmuch_database_get_config (db, "testkey2", &val)); printf("testkey2 = %s\n", val); } EOF @@ -60,7 +49,7 @@ test_begin_subtest "notmuch_database_get_config_list: empty list" cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} { notmuch_config_list_t *list; - RUN(notmuch_database_get_config_list (db, "nonexistent", &list)); + EXPECT0(notmuch_database_get_config_list (db, "nonexistent", &list)); printf("valid = %d\n", notmuch_config_list_valid (list)); notmuch_config_list_destroy (list); } @@ -77,9 +66,9 @@ test_begin_subtest "notmuch_database_get_config_list: all pairs" cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} { notmuch_config_list_t *list; - RUN(notmuch_database_set_config (db, "zzzafter", "afterval")); - RUN(notmuch_database_set_config (db, "aaabefore", "beforeval")); - RUN(notmuch_database_get_config_list (db, "", &list)); + EXPECT0(notmuch_database_set_config (db, "zzzafter", "afterval")); + EXPECT0(notmuch_database_set_config (db, "aaabefore", "beforeval")); + EXPECT0(notmuch_database_get_config_list (db, "", &list)); for (; notmuch_config_list_valid (list); notmuch_config_list_move_to_next (list)) { printf("%s %s\n", notmuch_config_list_key (list), notmuch_config_list_value(list)); } @@ -100,7 +89,7 @@ test_begin_subtest "notmuch_database_get_config_list: one prefix" cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} { notmuch_config_list_t *list; - RUN(notmuch_database_get_config_list (db, "testkey", &list)); + EXPECT0(notmuch_database_get_config_list (db, "testkey", &list)); for (; notmuch_config_list_valid (list); notmuch_config_list_move_to_next (list)) { printf("%s %s\n", notmuch_config_list_key (list), notmuch_config_list_value(list)); } @@ -118,7 +107,7 @@ test_expect_equal_file EXPECTED OUTPUT test_begin_subtest "dump config" cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} { - RUN(notmuch_database_set_config (db, "key with spaces", "value, with, spaces!")); + EXPECT0(notmuch_database_set_config (db, "key with spaces", "value, with, spaces!")); } EOF notmuch dump --include=config >OUTPUT @@ -136,7 +125,7 @@ test_begin_subtest "restore config" notmuch dump --include=config >EXPECTED cat c_head - c_tail <<'EOF' | test_C ${MAIL_DIR} { - RUN(notmuch_database_set_config (db, "testkey1", "mutatedvalue")); + EXPECT0(notmuch_database_set_config (db, "testkey1", "mutatedvalue")); } EOF notmuch restore --include=config <EXPECTED diff --git a/test/notmuch-test.h b/test/notmuch-test.h new file mode 100644 index 0000000..e6e1d6c --- /dev/null +++ b/test/notmuch-test.h @@ -0,0 +1,15 @@ +#ifndef _NOTMUCH_TEST_H +#define _NOTMUCH_TEST_H +#include <stdio.h> +#include <notmuch.h> + +void expect0(int line, notmuch_status_t ret) +{ + if (ret) { + fprintf (stderr, "line %d: %s\n", line, ret); + exit (1); + } +} + +#define EXPECT0(v) expect0(__LINE__, v); +#endif diff --git a/test/test-lib.sh b/test/test-lib.sh index 201d0eb..442d293 100644 --- a/test/test-lib.sh +++ b/test/test-lib.sh @@ -1199,7 +1199,7 @@ test_C () { test_file="${exec_file}.c" cat > ${test_file} export LD_LIBRARY_PATH=${TEST_DIRECTORY}/../lib - ${TEST_CC} ${TEST_CFLAGS} -I${TEST_DIRECTORY}/../lib -o ${exec_file} ${test_file} -L${TEST_DIRECTORY}/../lib/ -lnotmuch -ltalloc + ${TEST_CC} ${TEST_CFLAGS} -I${TEST_DIRECTORY} -I${TEST_DIRECTORY}/../lib -o ${exec_file} ${test_file} -L${TEST_DIRECTORY}/../lib/ -lnotmuch -ltalloc echo "== stdout ==" > OUTPUT.stdout echo "== stderr ==" > OUTPUT.stderr ./${exec_file} "$@" 1>>OUTPUT.stdout 2>>OUTPUT.stderr -- 2.8.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] fixup! test: factor out some boilerplate from C tests 2016-06-04 12:29 ` [PATCH 2/4] test: factor out some boilerplate from C tests David Bremner @ 2016-06-04 12:37 ` David Bremner 2016-06-14 0:57 ` [PATCH 2/4] " David Bremner 1 sibling, 0 replies; 13+ messages in thread From: David Bremner @ 2016-06-04 12:37 UTC (permalink / raw) To: David Bremner, Istvan Marko, Jani Nikula, notmuch --- test/notmuch-test.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/notmuch-test.h b/test/notmuch-test.h index e6e1d6c..d39febb 100644 --- a/test/notmuch-test.h +++ b/test/notmuch-test.h @@ -3,7 +3,8 @@ #include <stdio.h> #include <notmuch.h> -void expect0(int line, notmuch_status_t ret) +inline static void +expect0(int line, notmuch_status_t ret) { if (ret) { fprintf (stderr, "line %d: %s\n", line, ret); -- 2.8.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] test: factor out some boilerplate from C tests 2016-06-04 12:29 ` [PATCH 2/4] test: factor out some boilerplate from C tests David Bremner 2016-06-04 12:37 ` [PATCH] fixup! " David Bremner @ 2016-06-14 0:57 ` David Bremner 1 sibling, 0 replies; 13+ messages in thread From: David Bremner @ 2016-06-14 0:57 UTC (permalink / raw) To: notmuch David Bremner <david@tethera.net> writes: > The trick of having a common header file doesn't work to share between > test scripts, so make an include file in the test directory. I pushed this one, and the fixup, because it was starting to get confusing with various feature branches depending on it, or conflicting with it. d ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/4] test: initial tests for locking retry 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner 2016-06-04 12:29 ` [PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner 2016-06-04 12:29 ` [PATCH 2/4] test: factor out some boilerplate from C tests David Bremner @ 2016-06-04 12:29 ` David Bremner 2016-06-04 12:29 ` [PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK David Bremner 3 siblings, 0 replies; 13+ messages in thread From: David Bremner @ 2016-06-04 12:29 UTC (permalink / raw) To: Istvan Marko, Jani Nikula, notmuch 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner ` (2 preceding siblings ...) 2016-06-04 12:29 ` [PATCH 3/4] test: initial tests for locking retry David Bremner @ 2016-06-04 12:29 ` David Bremner 2016-06-04 12:38 ` [PATCH] fixup! " David Bremner 3 siblings, 1 reply; 13+ messages in thread From: David Bremner @ 2016-06-04 12:29 UTC (permalink / raw) To: Istvan Marko, Jani Nikula, notmuch Currently building with DB_RETRY_LOCK is dependent on xapian version, but I can imagine a configure switch to turn it off. --- lib/built-with.c | 2 ++ notmuch-config.c | 3 +++ 2 files changed, 5 insertions(+) diff --git a/lib/built-with.c b/lib/built-with.c index 7ea1d7f..9dd9e2d 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 c618f2c..16eaac9 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 -- 2.8.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH] fixup! lib: add built_with handling for XAPIAN_DB_RETRY_LOCK 2016-06-04 12:29 ` [PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK David Bremner @ 2016-06-04 12:38 ` David Bremner 0 siblings, 0 replies; 13+ messages in thread From: David Bremner @ 2016-06-04 12:38 UTC (permalink / raw) To: David Bremner, Istvan Marko, Jani Nikula, notmuch --- test/T030-config.sh | 1 + test/T040-setup.sh | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available 2016-05-03 15:09 [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available Istvan Marko 2016-05-03 16:31 ` Jani Nikula @ 2016-05-03 19:01 ` David Bremner 1 sibling, 0 replies; 13+ messages in thread From: David Bremner @ 2016-05-03 19:01 UTC (permalink / raw) To: Istvan Marko, notmuch Istvan Marko <notmuch@kismala.com> writes: > Xapian 1.3 has introduced the DB_RETRY_LOCK flag (Xapian bug > 275). Detect it in configure and use it if available. With this flag > commands that need the write lock will wait for their turn instead of > aborting when it's not immediately available. > --- > configure | 25 ++++++++++++++++++++++++- > lib/database.cc | 5 +++++ > 2 files changed, 29 insertions(+), 1 deletion(-) > > diff --git a/configure b/configure > index 6231d2b..0c1d5bc 100755 > --- a/configure > +++ b/configure > @@ -371,6 +371,21 @@ if [ ${have_xapian} = "1" ]; then > esac > fi > > +# DB_RETRY_LOCK is only supported on Xapian > 1.3.2 > +have_xapian_db_retry_lock=0 > +if [ ${have_xapian} = "1" ]; then > + printf "Checking for Xapian lock retry support... " > + case "${xapian_version}" in > + 0.*|1.[012].*|1.3.[0-2]) > + printf "No (only available with Xapian > 1.3.2).\n" ;; > + [1-9]*.[0-9]*.[0-9]*) > + have_xapian_db_retry_lock=1 > + printf "Yes.\n" ;; > + *) > + printf "Unknown version.\n" ;; > + esac > +fi First, thanks for writing the patch. I'd completely forgotten about this feature, and it's something pretty important for many notmuch users. I saw you got some good feedback from Jani. As a less urgent comment, what about writing the test in the style of http://article.gmane.org/gmane.mail.notmuch.general/22329 I don't know if there's complete concensus on this, but I personally much prefer testing features to versions when possible. d ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-06-14 0:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-03 15:09 [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available Istvan Marko 2016-05-03 16:31 ` Jani Nikula 2016-05-03 19:12 ` [PATCH v2] " Istvan Marko 2016-05-05 10:33 ` David Bremner 2016-06-04 12:29 ` v3 of DB_RETRY_LOCK David Bremner 2016-06-04 12:29 ` [PATCH 1/4] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner 2016-06-04 12:29 ` [PATCH 2/4] test: factor out some boilerplate from C tests David Bremner 2016-06-04 12:37 ` [PATCH] fixup! " David Bremner 2016-06-14 0:57 ` [PATCH 2/4] " David Bremner 2016-06-04 12:29 ` [PATCH 3/4] test: initial tests for locking retry David Bremner 2016-06-04 12:29 ` [PATCH 4/4] lib: add built_with handling for XAPIAN_DB_RETRY_LOCK David Bremner 2016-06-04 12:38 ` [PATCH] fixup! " David Bremner 2016-05-03 19:01 ` [PATCH] Use the Xapian::DB_RETRY_LOCK flag when available David Bremner
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).