reopen should re-read stub database file

[Richard Boulton]

Currently, when a database is opened via a stub db file (or directory containing a XAPIANDB stub file), calling reopen() on that database does not re-read the stub db file, it just reopens the databases which were listed in that file at the time of the first open.

This is probably desirable in some cases - if one of the databases listed in the stub db file is an inmemory database, for example. It's also somewhat helpful if the stub db file is not expected to change, since this avoids unwanted overhead in the call to reopen().

However, it's definitely undesirable in the case of a database produced by replication: the replication client creates a directory containing a XAPIANDB file which either points to a subdirectory called replica_0 or replica_1 - the subdirectory in use is "toggled" when a full database copy from the remote server is performed. Users of replication don't know about this detail, so would reasonably expect reopen() to simply given them the latest copy of the database retrieved by the replication client. Instead, if a single copy has occurred since opening, reopen() will return a DatabaseOpeningError, complaining that the database files are missing, and if a second copy has occurred, the reopen() will usually appear to succeed, but will later complain that the database is corrupt, since the database has been replaced by a completely new database.

My feeling is that reopen() is producing "suprising" behaviour here, from most users point of view, and that we should change it to re-read the stub database file. After all, one way to open a stub database is to pass its path to the Xapian::Database constructor - it behaves like a database, so calling reopen() ought to reopen the stub file and re-read it.

It might be nice to retain the existing behaviour for reopen() to allow users who fully understand the situation to avoid the overhead of re-reading the stub file. I'm not fully convinced the performance gain here outweights the benefit of a less confusing behaviour, but if we want to do that, one approach would be to have an optional parameter added to reopen() which avoids re-reading stub databases if set. Alternatively, there could be a flag set in a stub database somehow to indicate that the stub database shouldn't be re-read on reopen. I don't think this should be set by default, though.

My belief that the current behaviour is surprising is partly because I've been confused by it. I think that the cause of the confusion is twofold. Partly, the name of "reopen" implies (to me, anyway) that the result should be the same as closing the database and opening it again. For a replicated database, the stub file is "hidden", so the user will be surprised by this behaviour; but I was surprised by this behaviour even knowing about the internal details of stub databases. If the method was called "move_to_latest_revision" it might be easier to explain, (but only if users also understood the difference between a replication step which applied changesets, and one which performed a full database copy).

[Richard Boulton]

I've assigned this ticket to me - I'd be happy to put together an implementation which makes reopen() re-read the stub database if you're happy with that idea, Olly, but I'll hold off until we've discussed a bit.

[Olly Betts]

This seems a can of worms to me, and I think would be a mistake to aim to address it with a behavioural change before 1.2.0.

[Richard Boulton]

Hmm. Yes, seems a fair assessment. Probably something for the documentation for replication for now, then (and probably for the release notes for 1.2.0).

[Richard Boulton]

Added a note to the replication documentation (in r13884). Not sure which milestone this ticket should now be against - we'll leave any further work until 1.2.x, but I think it would be good to ensure we add a mention about this issue to the release notes.

[Richard Boulton]

Some partially written tests for this issue, which may (or may not) be helpful when implementing a fix for this.

[Richard Boulton]

I've added a release notes page for 1.2.0 to the wiki, and put a note about this issue in there, so there's nothing more to do for this ticket before 1.2.0. Marking it for the 1.2.x milestone.

[Olly Betts]

The patch fails the test for a short write() in write_stub(), which is likely to cause tests to fail for some platforms with some sizes of stub data, and could potentially cause random test failures. That really needs fixing.

I'm sure we must already have code to reliably create a stub file elsewhere in the testsuite.

Also, if you're going to add the same document repeatedly in a loop to build a database, create the Document object just once outside the loop. The point is to create the database for the tests we want to perform, not to test database creation, and the testsuite is already too slow. Needlessly creating and destroying an extra 2997 Document objects is overhead best avoided.

[Richard Boulton]

We had more discussion on this on IRC, but never came to any firm conclusion, IIRC.

I think it's reasonable to conclude that changing the implementation of reopen, and databases in general, such that stub database files get re-read on reopen is too intrusive to be worth the effort. The "reopen" name is potentially misleading here, but again, it's probably not worth trying to rename that (and anyway, I can't think of a much clearer name).

It would be nice to provide some way to reopen readers on replicated databases without having to close and open from scratch - but this isn't too hard to work around, and the plans for brass will include fixing this (by avoiding using a "hidden" stub database), so this shouldn't be a long term problem.

Therefore, I'm willing to accept the argument that the fix is just to leave the notes in the replication documentation about this, and close the ticket.

[Olly Betts]

OK, no arguments from me on that.

I've adjust the documentation to reflect this conclusion in trunk r14977. This isn't on 1.0, so nothing to backport.