[PATCH] lib: work around xapian bug with get_mset(0,0, x)

[PATCH] lib: work around xapian bug with get_mset(0,0, x)

[David Bremner]

At least Fedora28 triggers this Xapian bug due to some toolchain change .

   https://bugzilla.redhat.com/show_bug.cgi?id=1546162

The underlying bug is fixed in xapian commit f92e2a936c1592, and
should be fixed in Xapian 1.4.6
---

I have verified this doesn't break the test suite in my environment,
but I would appreciate feedback from someone with a Fedora 28 test
platform.

 lib/query.cc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/query.cc b/lib/query.cc
index d633fa3d..7fdf992d 100644
--- a/lib/query.cc
+++ b/lib/query.cc
@@ -652,9 +652,9 @@ _notmuch_query_count_documents (notmuch_query_t *query, const char *type, unsign
 	/*
 	 * Set the checkatleast parameter to the number of documents
 	 * in the database to make get_matches_estimated() exact.
-	 * Set the max parameter to 0 to avoid fetching documents we will discard.
+	 * Set the max parameter to 1 to avoid fetching documents we will discard.
 	 */
-	mset = enquire.get_mset (0, 0,
+	mset = enquire.get_mset (0, 1,
 				 notmuch->xapian_db->get_doccount ());
 
 	count = mset.get_matches_estimated();
-- 
2.16.3

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[Tomi Ollila]

On Fri, Apr 06 2018, David Bremner wrote:

> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>
> The underlying bug is fixed in xapian commit f92e2a936c1592, and
> should be fixed in Xapian 1.4.6
> ---
>
> I have verified this doesn't break the test suite in my environment,
> but I would appreciate feedback from someone with a Fedora 28 test
> platform.

Without this change, on Fedora 28 container:

9 aborts.
test execution halts at T570-revision-tracking.2 since ${result} 
is empty string

with this change: failure count is about the same as with fedora 27

so, +1

That, is my setup, pulled fedora:28 docker image and run a script
to install some stuff there. I'd bet I can get the error count
down by tuning the creation script...

Tomi

[PATCH] NEWS: news item for mset fix

[David Bremner]

---
 NEWS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/NEWS b/NEWS
index 39ce7707..05ecebaf 100644
--- a/NEWS
+++ b/NEWS
@@ -1,3 +1,14 @@
+Notmuch 0.26.2 (UNRELEASED)
+===========================
+
+Library Changes
+---------------
+
+Work around Xapian bug with `get_mset(0,0, x)`. This causes aborts in
+`_notmuch_query_count_documents` on e.g. Fedora 28.  The underlying bug
+is fixed in Xapian commit f92e2a936c1592, and should be fixed in
+Xapian 1.4.6.
+
 Notmuch 0.26.1 (2018-04-02)
 ===========================
 
-- 
2.17.0

Re: [PATCH] NEWS: news item for mset fix

[Tomi Ollila]

On Tue, Apr 17 2018, David Bremner wrote:

> ---
>  NEWS | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index 39ce7707..05ecebaf 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,3 +1,14 @@
> +Notmuch 0.26.2 (UNRELEASED)
> +===========================
> +
> +Library Changes
> +---------------
> +
> +Work around Xapian bug with `get_mset(0,0, x)`. This causes aborts in

Add space after comma (,)

> +`_notmuch_query_count_documents` on e.g. Fedora 28.  The underlying bug

One extra space after 28.

> +is fixed in Xapian commit f92e2a936c1592, and should be fixed in

could we be more confident and say 'will be fixed in'

> +Xapian 1.4.6.

Tomi

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[Daniel Kahn Gillmor]

On Fri 2018-04-06 08:43:07 -0300, David Bremner wrote:
> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>
> The underlying bug is fixed in xapian commit f92e2a936c1592, and
> should be fixed in Xapian 1.4.6

is there any way that we can apply this change only if we detect that
we're running with an unfixed version of Xapian?

      --dkg

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> On Fri 2018-04-06 08:43:07 -0300, David Bremner wrote:
>> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>>
>>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>>
>> The underlying bug is fixed in xapian commit f92e2a936c1592, and
>> should be fixed in Xapian 1.4.6
>
> is there any way that we can apply this change only if we detect that
> we're running with an unfixed version of Xapian?
>
>       --dkg

It's possible, but I think we'd need a new configure check, and some
ifdefs. We could consider doing that later, but I don't think it's the
right approach for a bugfix release.

d

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[Daniel Kahn Gillmor]

On Wed 2018-04-18 20:13:54 -0300, David Bremner wrote:
> Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:
>> On Fri 2018-04-06 08:43:07 -0300, David Bremner wrote:
>>> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>>>
>>>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>>>
>>> The underlying bug is fixed in xapian commit f92e2a936c1592, and
>>> should be fixed in Xapian 1.4.6
>>
>> is there any way that we can apply this change only if we detect that
>> we're running with an unfixed version of Xapian?
>
> It's possible, but I think we'd need a new configure check, and some
> ifdefs. We could consider doing that later, but I don't think it's the
> right approach for a bugfix release.

I was thinking of a runtime check, not a compile-time check -- to ensure
that we drop the workaround as soon as the library is upgraded beneath
us.

But if we want a compile-time check, i don't think we'd need anything
fancier than something like (potentially even directly in query.cc):

    #if XAPIAN_AT_LEAST(1,4,6)
    #define MSET_GET_MIN_COUNT 0
    #else
    #define MSET_GET_MIN_COUNT 1
    #endif

what else were you thinking we'd need?

     --dkg

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:


> But if we want a compile-time check, i don't think we'd need anything
> fancier than something like (potentially even directly in query.cc):
>
>     #if XAPIAN_AT_LEAST(1,4,6)
>     #define MSET_GET_MIN_COUNT 0
>     #else
>     #define MSET_GET_MIN_COUNT 1
>     #endif
>
> what else were you thinking we'd need?
>
>      --dkg

I forgot about the XAPIAN_AT_LEAST macro. Nonetheless, I'm not keen to
ship untested/uncompiled code (no matter how "obviously correct" it is),
so I'd prefer to wait until Xapian 1.4.6 actually exists before adding
an ifdef like that. FWIW I doubt that the performance impact of passing
1 instead of 0 here is measurable. For me, getting a bug fix release out
is a matter of some urgency; fine tuning and testing against xapian
1.4.6 can wait.

d

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[David Bremner]

David Bremner <david@tethera.net> writes:

> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>
> The underlying bug is fixed in xapian commit f92e2a936c1592, and
> should be fixed in Xapian 1.4.6

pushed to origin and master

d

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[David Bremner]

David Bremner <david@tethera.net> writes:

> David Bremner <david@tethera.net> writes:
>
>> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>>
>>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>>
>> The underlying bug is fixed in xapian commit f92e2a936c1592, and
>> should be fixed in Xapian 1.4.6
>
> pushed to origin and master

Ugh. To release and master.

d

Re: [PATCH] lib: work around xapian bug with get_mset(0,0, x)

[David Bremner]

David Bremner <david@tethera.net> writes:

> At least Fedora28 triggers this Xapian bug due to some toolchain change .
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=1546162
>
> The underlying bug is fixed in xapian commit f92e2a936c1592, and
> should be fixed in Xapian 1.4.6

pushed as part of 0.26.2 release