Stub databases should be read with msvc_posix_open

[Richard Boulton]

Currently, stub databases are read using a standard C++ ifstream. (See backends/database.cc, function open_stub()) This works fine, except that if a user (or the database replication code) tries, on Windows, to atomically rename a new stub db file over an existing one, it will receive an error if the old stub DB file was open.

This can be avoided if we instead use msvc_posix_open() (or just open() on unix) in open_stub() to get a file handle for the stub database, and access it using C file-handling routines.

[Olly Betts]

hmm, but if we do this, what happens on windows if the file is overwritten while we're reading it? The read will fail I believe...

[Richard Boulton]

So, a full fix would probably have to handle failures of the read, and retry. What a pain.

[Olly Betts]

Ah yes, that should work if the read returns a suitable error (which I think it does).

[Olly Betts]

For MSVC it looks like we could just write:

ifstream stub(msvc_posix_open(file.c_str(), O_RDONLY));

But GCC's libstdc++ doesn't have this non-standard form, so we can't use this for mingw, but there is stdio_filebuf, which allows you to wrap an fd or FILE* as an istream or ostream. This would allow us to keep the current code with only minor changes on the affected platform.

We should perhaps check if iostream imposes an overhead over the C FILE* routines, and if there is much of one have a policy to avoid istream and ostream instead.

Also while one option is to retry the whole stub read upon read failing, another is to simply require that the stub update attempt retries. It is perhaps better to have the writer blocked by heavy reader activity than have readers blocked by heavy writer activity as readers tend to be more performance sensitive.

[Olly Betts]

Milestone renamed

[Olly Betts]

While looking at fixing #854, we ideally want to read the stub file from a file descriptor. In the case where we've been asked to open a database passing in a path which is a file and that file is large enough that it could be a single file database (which means at least 2KB for glass and honey) we open an fd on it to look at the file magic to see if it is, and if not we currently close the fd and open the file again as a std::ifstream.

That means there's a race condition if you replace a >= 2KB stub file with a single file database - if you're very unlucky with the timing, a reader could sniff the stub file, close it, another process updates the filename to be a single file database, and the reader opens that as a std::ifstream and tries to parse it as a stub file.

I think there's also a race for a smaller stub file - as we could stat() the stub file, another process updates the filename to be a single file database, and the reader opens that as a std::ifstream and tries to parse it as a stub file. We should always open as a file descriptor first and use fstat() on that to decide if we can skip checking the file magic.

(It's also slightly inefficient to open the file twice, though I suspect most stub files are under 2KB so currently only get opened as a std::ifstream.)

That requires moving to reading stub files from a file descriptor, so as part of that we can address this issue by using posixy_open() (which replaced msvc_posix_open() back in 2012, and is just an alias to open() on Unix-like platforms). We probably just need to ensure that all reading of stub files happens this way (there's some in replication for example; ideally they should probably share more code anyway).