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

[Jani Nikula <jani@nikula.org>]

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