Xapian locking errors with custom query parser

Xapian locking errors with custom query parser

[Michal Sojka]

Hi Austin,

when I switched to using your custom query parser I started experiencing
"Unable to get write lock" errors when I run my initial tagging script.
I thought that this was because I run the script while processing the
mail in Emacs, but today I realized that this happens even without Emacs
and that it can be reproduced by a simple command below.

i=0; while notmuch tag +xxx tag:whatever; do i=$((i+1)); echo $i; done

It seems that sometimes the unlocking of the Xapian database happens
somewhat lazily and when the subsequent command starts I get:

A Xapian exception occurred opening database: Unable to get write lock on /home/wsh/mail/.notmuch/xapian: already locked

This happens almost regularly after a few iterations of the loop even if
the query doesn't match anything. 

Do you have an idea what may cause this?

Can you reproduce this on your system?

The version I use can be obtained from

  git://rtime.felk.cvut.cz/notmuch.git custom-query-pasrser

Thanks
-Michal

Re: Xapian locking errors with custom query parser

[Austin Clements]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

This should most definitely not happen.  I'll look in to it.

On Fri, Jan 28, 2011 at 5:18 AM, Michal Sojka <sojkam1@fel.cvut.cz> wrote:

> Hi Austin,
>
> when I switched to using your custom query parser I started experiencing
> "Unable to get write lock" errors when I run my initial tagging script.
> I thought that this was because I run the script while processing the
> mail in Emacs, but today I realized that this happens even without Emacs
> and that it can be reproduced by a simple command below.
>
> i=0; while notmuch tag +xxx tag:whatever; do i=$((i+1)); echo $i; done
>
> It seems that sometimes the unlocking of the Xapian database happens
> somewhat lazily and when the subsequent command starts I get:
>
> A Xapian exception occurred opening database: Unable to get write lock on
> /home/wsh/mail/.notmuch/xapian: already locked
>
> This happens almost regularly after a few iterations of the loop even if
> the query doesn't match anything.
>
> Do you have an idea what may cause this?
>
> Can you reproduce this on your system?
>
> The version I use can be obtained from
>
>  git://rtime.felk.cvut.cz/notmuch.git custom-query-pasrser
>
> Thanks
> -Michal
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch
>

[-- Attachment #2: Type: text/html, Size: 1871 bytes --]

Re: Xapian locking errors with custom query parser

[Austin Clements]

Looks like I managed to remove the line that deletes (and thus closes
and unlocks) the Xapian::Database object in a bout of overzealous code
removal.  The consequence was exactly what you suspected; there was a
brief window after notmuch had exited before the database actually got
unlocked.  Here's the fix.  cworth, what's the most convenient way for
me to slip this in to the patch series?

diff --git a/lib/database.cc b/lib/database.cc
index 7253bdf..3c488a9 100644
--- a/lib/database.cc
+++ b/lib/database.cc
@@ -826,6 +826,7 @@ notmuch_database_close (notmuch_database_t *notmuch)
     }

     delete notmuch->term_gen;
+    delete notmuch->xapian_db;
     talloc_free (notmuch);
 }


On Fri, Jan 28, 2011 at 11:35 AM, Austin Clements <amdragon@mit.edu> wrote:
> This should most definitely not happen.  I'll look in to it.
>
> On Fri, Jan 28, 2011 at 5:18 AM, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
>>
>> Hi Austin,
>>
>> when I switched to using your custom query parser I started experiencing
>> "Unable to get write lock" errors when I run my initial tagging script.
>> I thought that this was because I run the script while processing the
>> mail in Emacs, but today I realized that this happens even without Emacs
>> and that it can be reproduced by a simple command below.
>>
>> i=0; while notmuch tag +xxx tag:whatever; do i=$((i+1)); echo $i; done
>>
>> It seems that sometimes the unlocking of the Xapian database happens
>> somewhat lazily and when the subsequent command starts I get:
>>
>> A Xapian exception occurred opening database: Unable to get write lock on
>> /home/wsh/mail/.notmuch/xapian: already locked
>>
>> This happens almost regularly after a few iterations of the loop even if
>> the query doesn't match anything.
>>
>> Do you have an idea what may cause this?
>>
>> Can you reproduce this on your system?
>>
>> The version I use can be obtained from
>>
>>  git://rtime.felk.cvut.cz/notmuch.git custom-query-pasrser
>>
>> Thanks
>> -Michal
>> _______________________________________________
>> notmuch mailing list
>> notmuch@notmuchmail.org
>> http://notmuchmail.org/mailman/listinfo/notmuch
>
>

Re: Xapian locking errors with custom query parser

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

On Fri, 28 Jan 2011 21:26:03 -0500, Austin Clements <amdragon@mit.edu> wrote:
> unlocked.  Here's the fix.  cworth, what's the most convenient way for
> me to slip this in to the patch series?

I'd most prefer a rebased branch including the fix, along with an email
sent to me, (giving either the branch-name to pull or else the actual
patches via email).

[I think I answered this in IRC a while ago, but I don't see a later
email from you on this topic. Do you have a branch that's ready for me?
Perhaps that's qparser-3 (which looks more likely than qparser or
qparser-hack)]

Maybe we could use a naming convention for branches that people feel are
ready for me to pull. For example, if I look at Austin's current
git branches I see:

  amdragon/0.3.x
  amdragon/atomic-new-hack
  amdragon/atomic-new-v1
  amdragon/eager-metadata
  amdragon/eager-metadata-2
  amdragon/eager-metadata-v3
  amdragon/folder-hack
  amdragon/getdate
  amdragon/inheritable-tags-hack
  amdragon/master
  amdragon/qparser
  amdragon/qparser-3
  amdragon/qparser-hack
  amdragon/search-perf
  amdragon/search-perf-2
  amdragon/search-perf-3
  amdragon/search-perf-hack

Some of those are obviously old (search-perf and search-perf-2 obviously
superceded by search-perf-3). And some are obviously just experiements
(qparser-hack, search-perf-hack, folder-hack, and qparser-hack).

But the others are less clear. One option is for me to not go looking
for any branches except in response to specific pull requests. Another
option would be for people to name branches something like
for-cworth/qparser-3 once things are ready to go.

That way, I could do something like:

	git remote update
	git branch -r | grep for-cworth

and see what people are proposing.

What do you think?

Of course, I still want to have email so that everyone can follow along
at home, and it's easy to reply for patch review, etc.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Xapian locking errors with custom query parser

[Austin Clements]

Quoth Carl Worth on Mar 10 at  6:21 pm:
> On Fri, 28 Jan 2011 21:26:03 -0500, Austin Clements <amdragon@mit.edu> wrote:
> > unlocked.  Here's the fix.  cworth, what's the most convenient way for
> > me to slip this in to the patch series?
> 
> I'd most prefer a rebased branch including the fix, along with an email
> sent to me, (giving either the branch-name to pull or else the actual
> patches via email).
> 
> [I think I answered this in IRC a while ago, but I don't see a later
> email from you on this topic. Do you have a branch that's ready for me?
> Perhaps that's qparser-3 (which looks more likely than qparser or
> qparser-hack)]

Yes, qparser-3 is ready for you, and has this fix folded in to it (see
id:20110202050336.GB28537@mit.edu).

> *snip*
>
> Some of those are obviously old (search-perf and search-perf-2 obviously
> superceded by search-perf-3). And some are obviously just experiements
> (qparser-hack, search-perf-hack, folder-hack, and qparser-hack).

I'd been migrating towards x-hack for my constantly-rebased
development branches, and x-v1, x-v2, etc, for stable review branches.
In this system, the highest versioned branch would always be the one
to review and branches that weren't ready for review would only have a
-hack branch.  (Of course, it doesn't help that that repository has
branches from before I was doing this and is also a mirror of your
master repository, which mixes in the branches you have there.)

> But the others are less clear. One option is for me to not go looking
> for any branches except in response to specific pull requests. Another
> option would be for people to name branches something like
> for-cworth/qparser-3 once things are ready to go.
> 
> That way, I could do something like:
> 
> 	git remote update
> 	git branch -r | grep for-cworth
> 
> and see what people are proposing.
> 
> What do you think?

Interesting.  I could see this being useful for decluttering
superseded review branches, though that would require renaming
superseded branches, which always causes a mess.

> Of course, I still want to have email so that everyone can follow along
> at home, and it's easy to reply for patch review, etc.
> 
> -Carl

Re: Xapian locking errors with custom query parser

[Jameson Rollins]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

> What do you think?
> 
> Of course, I still want to have email so that everyone can follow along
> at home, and it's easy to reply for patch review, etc.

Hey, Carl.  I would say you should just wait for an email to the list
that says explicitly that "branch X at remote Y is ready to be merged".
I feel like that would be most analogous to the sending of patches
directly to the list.  It's also less work for you, since you wouldn't
have to go around fishing for branches to merge.  And the more we can
reduce your work load the better!

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: Xapian locking errors with custom query parser

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 3758 bytes --]

On Thu, 10 Mar 2011 21:47:30 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
> Yes, qparser-3 is ready for you, and has this fix folded in to it (see
> id:20110202050336.GB28537@mit.edu).

Thanks.

I've finally had a chance to start looking at this.

The first thing that caught my eye was this question:

> +/* XXX notmuch currently registers "tag" as an exclusive boolean
> + * prefix, which means queries like "tag:x tag:y" will return messages
> + * with tag x OR tag y.  Is this intentional? */

This isn't "intentional" in the sense that it is desired, no.

Our documentation for the search syntax says:

    In addition to individual terms, multiple terms can  be  combined  with
    Boolean  operators  ( and, or, not , etc.). Each term in the query will
    be implicitly connected by a logical AND if  no  explicit  operator  is
    provided,  (except  that  terms with a common prefix will be implicitly
    combined with OR until we get Xapian defect #402 fixed).

So, when I originally wrote this code, the add_boolean_prefix function
didn't have the "exclusive" parameter that it has now. So that's
something to fix.

The next thing I notice is quite a lot of concern in the testing for
whether things were precisely Xapian compatible or not. I have two
different opinions about this:

1. For "new" search features (ADJ,NEAR,etc.) I do not have a strong
   interest in compatibility with Xapian.

   I was very careful when I wrote the documentation for the notmuch
   search syntax to only document features that I had used and tested,
   and that I was sure I wanted. (I was already thinking forward to
   perhaps writing a custom query parser at some point.)

   So you should really use our existing documentation as the
   guide. Please implement and test what it says.

   Beyond that, if you want to add additional features not mentioned in
   our documentation, then feel free to, and there's no good reason not
   to be Xapian compatible. But I also don't think there's a strong
   reason that we have to be compatible.

   Of course, for any new features here I would also like to see the
   documentation be updated.

2. For term splitting I do have a strong interest in Xapian compatibility.

   The difference here is that we aren't doing our own indexing, but
   instead relying on Xapian to do that for us, and we have also never
   carefully documented how the term splitting happens.

   What I want to happen here is that if a user grabs a chunk of text
   from an email, (say, "x#y"), and searches for it, that notmuch will
   find emails that actually contain that text. So if the indexer and
   the query parser disagree about something like this, then notmuch can
   break badly.

   I don't know how well notmuch currently meets that requirement, but
   I've been trusting in consistent term-splitting in the indexer and
   query-parser to help with this. So the frequent comments about
   incompatibility along these lines in your patches make me nervous.

   Can you enlighten me more about the compatibility differences in this
   area, and how things might break here?

> Interesting.  I could see this being useful for decluttering
> superseded review branches, though that would require renaming
> superseded branches, which always causes a mess.

Deleting any superseded for-cworth branch would never cause me any
problem. If you had other consumers of your branches that wouldn't be as
happy with branch names disappearing, then you might want to just let
them have another name outside the "for-cworth" space.

Anyway, it's just one idea for helping me get some more information from
git.

-Carl

-- 
carl.d.worth@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Xapian locking errors with custom query parser

[Michal Sojka]

On Fri, 11 Mar 2011, Carl Worth wrote:
> I've finally had a chance to start looking at this.

[...]

> 1. For "new" search features (ADJ,NEAR,etc.) I do not have a strong
>    interest in compatibility with Xapian.
> 
>    I was very careful when I wrote the documentation for the notmuch
>    search syntax to only document features that I had used and tested,
>    and that I was sure I wanted. (I was already thinking forward to
>    perhaps writing a custom query parser at some point.)
> 
>    So you should really use our existing documentation as the
>    guide. Please implement and test what it says.
> 
>    Beyond that, if you want to add additional features not mentioned in
>    our documentation, then feel free to, and there's no good reason not
>    to be Xapian compatible. But I also don't think there's a strong
>    reason that we have to be compatible.

Additionally, I'd suggest to support value range queries for dates with
".." syntax. Besides that some users may relay on this syntax, I use
date searches a lot and with custom query parser I have to type
"after:yesterday", which is unnecessarily long. I wish that
"yesterday..", which is much easier to type, would do the same.
Similarly, "mon..wed" would be easier to type than "after:mon
before:wed". What do you think?

-Michal

Re: Xapian locking errors with custom query parser

[Austin Clements]

I haven't made any changes to the query parser yet, but I wanted to
reply to your questions.  (I might not get a chance to change things
for a while; I spent this weekend catching my breath and dealing with
all of the things I punted over the past few weeks and tomorrow it's
back to a different grind.  Whoever said being a grad student was
hitting the snooze button on life was a liar.)

Responses inline.

On Fri, Mar 11, 2011 at 12:26 AM, Carl Worth <cworth@cworth.org> wrote:
> On Thu, 10 Mar 2011 21:47:30 -0500, Austin Clements <amdragon@MIT.EDU> wrote:
>> Yes, qparser-3 is ready for you, and has this fix folded in to it (see
>> id:20110202050336.GB28537@mit.edu).
>
> Thanks.
>
> I've finally had a chance to start looking at this.
>
> The first thing that caught my eye was this question:
>
>> +/* XXX notmuch currently registers "tag" as an exclusive boolean
>> + * prefix, which means queries like "tag:x tag:y" will return messages
>> + * with tag x OR tag y.  Is this intentional? */
>
> This isn't "intentional" in the sense that it is desired, no.
>
> Our documentation for the search syntax says:
>
>    In addition to individual terms, multiple terms can  be  combined  with
>    Boolean  operators  ( and, or, not , etc.). Each term in the query will
>    be implicitly connected by a logical AND if  no  explicit  operator  is
>    provided,  (except  that  terms with a common prefix will be implicitly
>    combined with OR until we get Xapian defect #402 fixed).
>
> So, when I originally wrote this code, the add_boolean_prefix function
> didn't have the "exclusive" parameter that it has now. So that's
> something to fix.

Okay.  I suppose that this applies to all boolean prefixes, then, and
not just tag.  That actually simplifies parse_prob, since it can treat
boolean prefixed terms just like other terms and won't have to match
up identical prefixes.

I suppose a separate patch should first change the boolean prefixes to
exclusive to make sure the custom parser is a drop-in replacement.

> The next thing I notice is quite a lot of concern in the testing for
> whether things were precisely Xapian compatible or not. I have two
> different opinions about this:
>
> 1. For "new" search features (ADJ,NEAR,etc.) I do not have a strong
>   interest in compatibility with Xapian.
>
>   I was very careful when I wrote the documentation for the notmuch
>   search syntax to only document features that I had used and tested,
>   and that I was sure I wanted. (I was already thinking forward to
>   perhaps writing a custom query parser at some point.)
>
>   So you should really use our existing documentation as the
>   guide. Please implement and test what it says.
>
>   Beyond that, if you want to add additional features not mentioned in
>   our documentation, then feel free to, and there's no good reason not
>   to be Xapian compatible. But I also don't think there's a strong
>   reason that we have to be compatible.
>
>   Of course, for any new features here I would also like to see the
>   documentation be updated.

I guess I didn't know what "etc" meant in the list of supported
boolean operators, so I took it to mean "whatever Xapian does".  I
leaned hard on Xapian compatibility because I didn't want to beak
anybody's setup, but I'm happy to strip out compatibility stuff
(especially NEAR and ADJ; those add a lot of complexity!)

Besides NEAR and ADJ, the only features I can think of that aren't in
the documentation but that I implemented are + and -.  But I think a
lot of people use these and they're really handy, so perhaps they
should be documented instead of being stripped.

> 2. For term splitting I do have a strong interest in Xapian compatibility.
>
>   The difference here is that we aren't doing our own indexing, but
>   instead relying on Xapian to do that for us, and we have also never
>   carefully documented how the term splitting happens.
>
>   What I want to happen here is that if a user grabs a chunk of text
>   from an email, (say, "x#y"), and searches for it, that notmuch will
>   find emails that actually contain that text. So if the indexer and
>   the query parser disagree about something like this, then notmuch can
>   break badly.
>
>   I don't know how well notmuch currently meets that requirement, but
>   I've been trusting in consistent term-splitting in the indexer and
>   query-parser to help with this. So the frequent comments about
>   incompatibility along these lines in your patches make me nervous.
>
>   Can you enlighten me more about the compatibility differences in this
>   area, and how things might break here?

In some sense, the term splitting the custom query parser does is more
Xapian-compatible than the Xapian parser's term splitting.  The custom
query parser uses the exact same TermGenerator that notmuch uses to
split documents in the first place, so the query term splitting will
be identical.  In fact, your x#y example *won't* do what you want with
the Xapian parser (it will be equivalent to x AND y), but it will with
the custom parser (it will be like "x y", which is as close as it can
get since Xapian doesn't index the #).

The real difference between the Xapian parser and the custom parser
lies in how they split *implicit phrases*.  In the Xapian parser,
characters that split terms are treated in one of two different ways:
they can either just split a term, but keep the resulting terms
together in a phrase; or they separate phrases '#' does the latter,
which is why x#y becomes x AND y.  In the custom parser, only
whitespace, '(', ')' and '"' separate phrases, and each phrase is then
split into terms using the TermGenerator.  Hence, the terms you get
are Xapian-compatible, but the custom parser treats more things as
phrases (using much more understandable rules).

I hope that addresses your concern with the term splitting.  If not,
please let me know.

Re: Xapian locking errors with custom query parser

[Austin Clements]

On Fri, Mar 11, 2011 at 3:58 AM, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> On Fri, 11 Mar 2011, Carl Worth wrote:
>> I've finally had a chance to start looking at this.
>
> [...]
>
>> 1. For "new" search features (ADJ,NEAR,etc.) I do not have a strong
>>    interest in compatibility with Xapian.
>>
>>    I was very careful when I wrote the documentation for the notmuch
>>    search syntax to only document features that I had used and tested,
>>    and that I was sure I wanted. (I was already thinking forward to
>>    perhaps writing a custom query parser at some point.)
>>
>>    So you should really use our existing documentation as the
>>    guide. Please implement and test what it says.
>>
>>    Beyond that, if you want to add additional features not mentioned in
>>    our documentation, then feel free to, and there's no good reason not
>>    to be Xapian compatible. But I also don't think there's a strong
>>    reason that we have to be compatible.
>
> Additionally, I'd suggest to support value range queries for dates with
> ".." syntax. Besides that some users may relay on this syntax, I use
> date searches a lot and with custom query parser I have to type
> "after:yesterday", which is unnecessarily long. I wish that
> "yesterday..", which is much easier to type, would do the same.
> Similarly, "mon..wed" would be easier to type than "after:mon
> before:wed". What do you think?

Personally, I just don't understand the .. range syntax, which is why
I left it out (also, I was following the example in the TODO file).
It's completely inconsistent with the rest of the query syntax and
makes no indication of what it's a range over (what if you had other
ordinal values to search over?  what if you could search by the
received date or the sent date?).

What about something like "date:mon..wed"?  That's consistent with the
query syntax (the range part becomes part of the date syntax, not part
of the top-level query syntax), it indicates the domain of the search
term in a clean and extensible way, and it's succinct.

Re: Date ranges syntax (was: Xapian locking errors with custom query parser)

[Michal Sojka]

On Mon, 21 Mar 2011, Austin Clements wrote:
> On Fri, Mar 11, 2011 at 3:58 AM, Michal Sojka <sojkam1@fel.cvut.cz> wrote:
> > Additionally, I'd suggest to support value range queries for dates with
> > ".." syntax. Besides that some users may relay on this syntax, I use
> > date searches a lot and with custom query parser I have to type
> > "after:yesterday", which is unnecessarily long. I wish that
> > "yesterday..", which is much easier to type, would do the same.
> > Similarly, "mon..wed" would be easier to type than "after:mon
> > before:wed". What do you think?
> 
> Personally, I just don't understand the .. range syntax, which is why
> I left it out (also, I was following the example in the TODO file).
> It's completely inconsistent with the rest of the query syntax and
> makes no indication of what it's a range over (what if you had other
> ordinal values to search over?  what if you could search by the
> received date or the sent date?).
> 
> What about something like "date:mon..wed"?  That's consistent with the
> query syntax (the range part becomes part of the date syntax, not part
> of the top-level query syntax), it indicates the domain of the search
> term in a clean and extensible way, and it's succinct.

Yes, the date prefix with ranges in value sounds reasonable and the word
"date" is even shorter than "after" or "before".

-Michal