notmuch dump: taking write-lock to protect from concurrent (cronned) notmuch new?

notmuch dump: taking write-lock to protect from concurrent (cronned) notmuch new?

[Maarten Aertsen]

Hi everyone,

(summary of IRC-conversation just now)
I did:
  - run notmuch new (and afew -t) in cron, every two minutes
  - run notmuch dump in cron, every 12 hours

I expected:
  - notmuch dump to complete, possibly causing notmuch new to fail in the meantime

I observed:
  - notmuch dump terminating with:
"terminate called after throwing an instance of 'Xapian::DatabaseModifiedError'"

mjw1009 suggested to change NOTMUCH_DATABASE_MODE_READ_ONLY on line
215 of notmuch-dump.c to NOTMUCH_DATABASE_MODE_READ_WRITE

I'm wondering if this hits enough people to motivate the addition of a
command line switch (or perhaps even a change in default behaviour?)

Thanks for writing this software.

best regards, Maarten

Re: notmuch dump: taking write-lock to protect from concurrent (cronned) notmuch new?

[Mark Walters]

On Fri, 06 Jun 2014, Maarten Aertsen <sagi-notmuch@rtsn.nl> wrote:
> Hi everyone,
>
> (summary of IRC-conversation just now)
> I did:
>   - run notmuch new (and afew -t) in cron, every two minutes
>   - run notmuch dump in cron, every 12 hours
>
> I expected:
>   - notmuch dump to complete, possibly causing notmuch new to fail in the meantime
>
> I observed:
>   - notmuch dump terminating with:
> "terminate called after throwing an instance of 'Xapian::DatabaseModifiedError'"
>
> mjw1009 suggested to change NOTMUCH_DATABASE_MODE_READ_ONLY on line
> 215 of notmuch-dump.c to NOTMUCH_DATABASE_MODE_READ_WRITE
>
> I'm wondering if this hits enough people to motivate the addition of a
> command line switch (or perhaps even a change in default behaviour?)

I think this is a clear bug but the fix is a little unclear. The above
fix works but it breaks one of the tests: "unicode message-ids" in
T150-tagging.sh.

I think the problem is that it does 
notmuch dump | sed... | notmuch restore

and if we open the dump read-write the restore will fail.

We can obviously fix the test, but I don't know if anyone is using a
pipe of this sort in their scripts. If this a likely problem then we
could offer a --no-lock option (which keeps the behaviour as now)

What do people think?

Best wishes

Mark

Re: notmuch dump: taking write-lock to protect from concurrent (cronned) notmuch new?

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:


>> mjw1009 suggested to change NOTMUCH_DATABASE_MODE_READ_ONLY on line
>> 215 of notmuch-dump.c to NOTMUCH_DATABASE_MODE_READ_WRITE
>>
>> I'm wondering if this hits enough people to motivate the addition of a
>> command line switch (or perhaps even a change in default behaviour?)
>
> I think this is a clear bug but the fix is a little unclear. The above
> fix works but it breaks one of the tests: "unicode message-ids" in
> T150-tagging.sh.
>
> I think the problem is that it does 
> notmuch dump | sed... | notmuch restore
>

My first reaction was "argh, we should be locking things less, not
more".  But then I read 

        http://getting-started-with-xapian.readthedocs.org/en/latest/xapian-core-rst/admin_notes.html?highlight=backup#id10

and now I'm not so sure, maybe write lock for dump is the right answer.

It seems hard to do anything sensible with "Database.reopen" in the
context of a backup.

Re: notmuch dump: taking write-lock to protect from concurrent (cronned) notmuch new?

[Mark Walters]

Hi

On Thu, 12 Jun 2014, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>
>>> mjw1009 suggested to change NOTMUCH_DATABASE_MODE_READ_ONLY on line
>>> 215 of notmuch-dump.c to NOTMUCH_DATABASE_MODE_READ_WRITE
>>>
>>> I'm wondering if this hits enough people to motivate the addition of a
>>> command line switch (or perhaps even a change in default behaviour?)
>>
>> I think this is a clear bug but the fix is a little unclear. The above
>> fix works but it breaks one of the tests: "unicode message-ids" in
>> T150-tagging.sh.
>>
>> I think the problem is that it does 
>> notmuch dump | sed... | notmuch restore
>>
>
> My first reaction was "argh, we should be locking things less, not
> more".  But then I read 
>
>         http://getting-started-with-xapian.readthedocs.org/en/latest/xapian-core-rst/admin_notes.html?highlight=backup#id10
>
> and now I'm not so sure, maybe write lock for dump is the right answer.

On irc Olly said

"I'd suggest locking the db by opening it R/W for the dump at least
until you can use reader locking to keep the read revision valid, but
it'll be a while before that's in a stable release"

and he also said that pipes of the form notmuch dump | ... notmuch
restore will probably fail if they change many tags.

So it is probably the way to go. But it does run the risk of breaking
some peoples (already fragile) scripts.

> It seems hard to do anything sensible with "Database.reopen" in the
> context of a backup.

Yes I agree.

Best wishes

Mark

[PATCH] dump: make dump take Xapian write lock

[Mark Walters]

Dump currently only takes the read lock. Xapian can cope with some
changes while maintaining a read snapshot but with more changes it
fails. Currently notmuch just gives a xapian error.

To avoid this we take the write lock when dumping. This prevents other
notmuch processes from modifying the xapian database preventing this
error.

Discussion with Olly on irc indicates that this is currently the best
solution: in xapian trunk there may be better possibilities using
snapshots but they need to make it to a release and propogate out to
users before we can switch approach.

Finally, this breaks one use case: pipelines of the form

notmuch dump | ... | notmuch restore

According to Olly this is already very fragile: it will only work on
small databases. One of the tests relies on this behaviour so fix that
to store the dump rather than use a pipe.
---
I haven't tested this much: all tests pass (with the one fix) and it is simple.

Best wishes

Mark


 notmuch-dump.c       |    2 +-
 test/T150-tagging.sh |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/notmuch-dump.c b/notmuch-dump.c
index 887a208..9c6ad7f 100644
--- a/notmuch-dump.c
+++ b/notmuch-dump.c
@@ -212,7 +212,7 @@ notmuch_dump_command (notmuch_config_t *config, int argc, char *argv[])
     int ret;
 
     if (notmuch_database_open (notmuch_config_get_database_path (config),
-			       NOTMUCH_DATABASE_MODE_READ_ONLY, &notmuch))
+			       NOTMUCH_DATABASE_MODE_READ_WRITE, &notmuch))
 	return EXIT_FAILURE;
 
     char *output_file_name = NULL;
diff --git a/test/T150-tagging.sh b/test/T150-tagging.sh
index dc118f3..45471ac 100755
--- a/test/T150-tagging.sh
+++ b/test/T150-tagging.sh
@@ -247,8 +247,8 @@ ${TEST_DIRECTORY}/random-corpus --config-path=${NOTMUCH_CONFIG} \
 notmuch dump --format=batch-tag | sed 's/^.* -- /+common_tag -- /' | \
     sort > EXPECTED
 
-notmuch dump --format=batch-tag | sed 's/^.* -- /  -- /' | \
-    notmuch restore --format=batch-tag
+notmuch dump --format=batch-tag | sed 's/^.* -- /  -- /' > INTERMEDIATE_STEP
+notmuch restore --format=batch-tag < INTERMEDIATE_STEP
 
 notmuch tag --batch < EXPECTED
 
-- 
1.7.10.4

Re: [PATCH] dump: make dump take Xapian write lock

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

>
> Discussion with Olly on irc indicates that this is currently the best
> solution: in xapian trunk there may be better possibilities using
> snapshots but they need to make it to a release and propogate out to
> users before we can switch approach.

I agree that this seems to be the only feasible approach to make dump
atomic.  I'm a little unsure about the benefits to the end user though.
Currently, the user has to check the return code of dump to ensure it
completed correctly.  With this change, the user will still have to
check that dump did not error out when trying to acquire a write lock.
The following ticket

      http://trac.xapian.org/ticket/275

suggests that if we wanted to a blocking open, we'd be on our own.

Did I miss something here about the benefits? I agree that failing
earlier is nicer, but is that it?

d

Re: [PATCH] dump: make dump take Xapian write lock

[Mark Walters]

On Tue, 15 Jul 2014, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>>
>> Discussion with Olly on irc indicates that this is currently the best
>> solution: in xapian trunk there may be better possibilities using
>> snapshots but they need to make it to a release and propogate out to
>> users before we can switch approach.
>
> I agree that this seems to be the only feasible approach to make dump
> atomic.  I'm a little unsure about the benefits to the end user though.
> Currently, the user has to check the return code of dump to ensure it
> completed correctly.  With this change, the user will still have to
> check that dump did not error out when trying to acquire a write lock.
> The following ticket
>
>       http://trac.xapian.org/ticket/275
>
> suggests that if we wanted to a blocking open, we'd be on our own.
>
> Did I miss something here about the benefits? I agree that failing
> earlier is nicer, but is that it?

You are correct that we may still fail. I think there are two
advantages in addition to failing earlier. 

First, I think that if the dump fails there will not be any output file:
I think the old version would leave a partial dump (note I haven't
checked that carefully). This makes it more likely the failing dump is
noticed.

Secondly, this version will only fail if the database is being written
to when it starts: the old version would fail if something attempted to
write to it at any point during the dump, which can be relatively
lengthy. This should at least reduce the frequency of problems.

Best wishes

Mark

Re: [PATCH] dump: make dump take Xapian write lock

[David Bremner]

Mark Walters <markwalters1009@gmail.com> writes:

> Dump currently only takes the read lock. Xapian can cope with some
> changes while maintaining a read snapshot but with more changes it
> fails. Currently notmuch just gives a xapian error.
>
> To avoid this we take the write lock when dumping. This prevents other
> notmuch processes from modifying the xapian database preventing this
> error.

pushed. This needs a NEWS item.

d