Re: [PATCH v2 42/43] refs: add LMDB refs backend

[Michael Haggerty <mhagger@alum.mit.edu>]

On 10/07/2015 03:51 AM, David Turner wrote:
> On Mon, 2015-10-05 at 17:47 +0200, Michael Haggerty wrote:
>> On 09/29/2015 12:02 AM, David Turner wrote:
>>> Add a database backend for refs using LMDB.  [...]
>>
>> I think you have said before that if one writer holds the write lock on
>> the DB, then other writers fail immediately. Is that correct? If so, is
>> that something that can be adjusted? I think it would be preferable for
>> the second writer to retry acquiring the write lock for a little while
>> with a timeout (as we now do when trying to acquire the packed-refs
>> lock). Otherwise you could have the unhappy situation that somebody
>> spends a long time pushing a packfile to a server, only to have the
>> reference update be rejected at the last moment due to a lock conflict
>> with another process that was touching completely different references.
>> We already do before/after consistency checks when updating references,
>> so you wouldn't even have to add such code in the backend yourself.
> 
> No, the second writer waits for the first writer to unlock (or for it to
> crash).

Cool, that's better behavior.

> [...]
>> Do you store "peeled" reference values for tags, as is done in
>> packed-refs? I think that is an important optimization.
> 
> No.  Do you happen to know in what situations this is a performance
> benefit, so that I can benchmark?  I suspect it would matter much less
> for the LMDB backend, because lookups are pretty quick.

The reference lookup speed is not relevant here. "Peeling" is applied to
references that point at tag objects (a.k.a. annotated tags). It means
that the tag object is looked up to see what *it* points at (recursively
if necessary) and the result is stored to the packed-refs file in a
specially-formatted extra line that looks like

    17f9f635c101aef03874e1de1d8d0322187494b3 refs/tags/v2.6.0
    ^be08dee9738eaaa0423885ed189c2b6ad8368cf0

I think a good command to benchmark would be `git show-refs -d` in a
repository with a number of annotated tags. This command's output is
similar to the output of `git ls-remote <remote>` and also comes up
during reference negotiation when fetching (so its performance is
definitely not moot).

> [...]
>> Currently we discard the reflog for a reference when the reference is
>> deleted. [...]
>> Have you thought about removing this limitation in the lbdb backend?
> 
> I'm going for feature parity first.  We can always add new functionality
> later.  This particular function would be pretty straightforward to add,
> I think.

+1

> [...]
>>> +The rsync and file:// transports don't work yet, because they
>>> +don't use the refs API.
>>
>> Do they fail gracefully?
> 
> Not particularly gracefully.
> 
> rsync: link_stat "/home/dturner/git/t/trash
> directory.t5510-fetch/.git/packed-refs" failed: No such file or
> directory (2)
> rsync error: some files/attrs were not transferred (see previous errors)
> (code 23) at main.c(1183) [sender=3.1.1]
> fatal: Could not run rsync to get refs
> -------------
> 
> The problem is that rsync on the client assumes that packed-refs exists.
> We could hack it to also check for refdb.

I guess this is something that will have to be improved sooner or later,
though I guess not as a precondition for merging this patch series.

> [...]
>> I'm somewhat surprised that you only register the lmdb backend if it is
>> used in the main repo. I would expect the backend to be registered
>> unconditionally on startup. The cost is trivial, isn't it?
> 
> Yeah, but this was the easiest place to do it.

OK.

> [...]

I'm really happy about your work.

Regarding strategy: I think a good approach would be to get as much of
the preparatory work as possible (the abstraction and separation of
refs-be-files) to the point where it can be merged before there is too
much more code churn in the area. That work is not very controversial, I
think, and letting it wait for a long time will increase the likelihood
of conflicts with other people's changes. The refs-be-lmdb patches, on
the other hand, (1) will take longer to get polished, (2) will take
longer to review because other people are not familiar with LDMB, and
(3) won't bitrot very fast anyway because they don't overlap as much
with areas that other people are likely to work on. So I would advocate
working on those at a more deliberate pace and planning for them to be
merged as a separate batch.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu