On Wed, Dec 02 2020, Han-Wen Nienhuys wrote:

On Wed, Dec 2, 2020 at 1:32 PM Johannes Schindelin
[off-list ref] wrote:

quoted

quoted

Consider processes P1 and P2, and the following sequence of actions

P1 opens ref DB (ie. opens a set of *.ref files for read)
P2 opens ref DB, executes a transaction. Post-transaction, it compacts
the reftable stack.
P2 exits
P1 exits

Currently, the compaction in P2 tries to delete the files obviated by
the compaction. On Windows this currently fails, because you can't
delete open files.

Indeed. So the design needs to be fixed, if it fails.

quoted

There are several options:

1) P2 should fail the compaction. This is bad because it will lead to
degraded performance over time, and it's not obvious if you can
anticipate that the deletion doesn't work.
2) P2 should retry deleting until it succeeds. This is bad, because a
reader can starve writers.
3) on exit, P1 should check if its *.ref files are still in use, and
delete them. This smells bad, because P1 is a read-only process, yet
it executes writes. Also, do we have good on-exit hooks in Git?
4) On exit, P1 does nothing. Stale *.ref files are left behind. Some
sort of GC process cleans things up asynchronously.
5) The ref DB should not keep files open, and should rather open and
close files as needed; this means P1 doesn't keep files open for long,
and P2 can retry safely.

I think 3) seems the cleanest to me (even though deleting in read
process feels weird), but perhaps we could fallback to 5) on windows
as well.

Traditionally, Git would fail gracefully (i.e. with a warning) to delete
the stale files, and try again at a later stage (during `git gc --auto`,
for example, or after the next compaction step).

So, how does this sound:

* add a void

  set_warn_handler(void(*handler)(char const*))

The handler is called for non-fatal errors (eg. deleting an in-use
.ref file during compaction), and can provide the warning.

* all .ref files will be prefixed with an 8-byte random string, to
avoid that new *.ref files collide with unreferenced, stale *.ref
files.

Just trying to follow along here. Do you mean prefix the file name or
the content of those *.ref files? In any case isn't this synonymous with
proposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just
this change?). The current spec seems to preclude prefixing either the
file content or filename, but maybe I'm misreading it:

https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions
https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header

When the reftable format was initially being discussed I suggested
somewhat flippantly (why not SQLite?). Partially because in trying to
invent what's essentially a portable and concurrently updated database
format we'd be likely to reinvent much of the same wheel.

So not to drag that whole thing up again as a proposal for a format
replacement, but I wonder what SQLite would do in this scenario & others
where desired DB semantics / transactions and FS/portability semantics
clash. AFAIK the reftable has only been battle-tested in production
under JGit so far (presumably Linux-only). Whereas SQLite has been
ported to and widely used on Windows, HP/UX and probably other systems
where Linux-specific assumptions don't apply.

* in reftable_stack_close(), after closing files, we check if *.ref
files we were reading have gone out of use. If so, delete those .ref
files, printing a warning on EACCESS.

* an on-exit handler in git calls reftable_stack_close()

* provide a reftable_stack_gc(), which loads tables.list and then
deletes all unused *.ref files that are older than tables.list.

quoted

quoted

What errno code does deleting an in-use file on Windows produce?

I believe it would be `EACCES`. See
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160
for the documented behavior (I believe that an in-use file is treated the
same way as a read-only file in this instance).

your commit message says "to avoid the prompt complaining that a
`.ref` file could not be deleted on Windows." AFAICT, this prompt is
coming from windows itself, because the reftable library doesn't issue
this type of warning. Is there a way to delete a file that just
returns EACCESS if it fails, without triggering a prompt? Or is the
prompt part of the "failing gracefully" behavior?

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.