Subject: Re: [RFC/WIP] Pluggable reference backends

From: Michael Haggerty <[email-masked]>
Date: Wed, 12 Mar 2014 12:43:35 +0100

Karsten,

Thanks for your feedback!

On 03/11/2014 11:56 AM, Karsten Blees wrote:
> Am 10.03.2014 12:00, schrieb Michael Haggerty:
>> 
>> Reference transactions ----------------------
> 
> Very cool ideas indeed.
> 
> However, I'm concerned a bit that transactions are conceptual
> overkill. How many concurrent updates do you expect in a repository?
> Wouldn't a single repo-wide lock suffice (and be _much_ simpler to
> implement with any backend, esp. file-based)?

I am mostly thinking about long-running processes, like "gc" and
"prune-refs", which need to be made race-free without blocking other
processes for the whole time they are running (whereas it might be quite
tolerable to have them fail or only complete part of their work in any
given invocation).  Also, I work at GitHub, where we have quite a few
repositories, some of which are quite active :-)

Remember that I'm not yet proposing anything like hard-core ACID
reference transactions.  I'm just clearing the way for various possible
changes in reference handling.  I listed the ideas only to whet people's
appetites and motivate the refactoring, which will take a while before
it bears any real fruit.

> The API you posted in [1] doesn't look very much like a transaction
> API either (rather like batch-updates). E.g. there's no rollback, the
> queue* methods cannot report failure, and there's no way to read a
> ref as part of the transaction. So I'm afraid that backends that
> support transactions out of the box (e.g. RDBMSs) will be hard to
> adapt to this.

Gmane is down at the moment but I assume you are referring to my patch
series and the ref_transaction implementation therein.

No explicit rollback is necessary at this stage, because the "commit"
function first locks all of the references that it wants to change
(first verifying that they have the expected values), and then modifies
them all.  By the time the references are locked, the whole transaction
is guaranteed to succeed [1].  If the locks can't all be acquired, then
any locks that were obtained are released.

If a caller wants to rollback a transaction, it only needs to free the
transaction instead of committing.  I should probably make that clearer
by renaming free_ref_transaction() to rollback_ref_transaction().  By
the time we start implementing other reference backends, that function
will of course have to do more.  For that matter, maybe
create_ref_transaction() should be renamed to begin_ref_transaction().
Now would be a good time for concrete bikeshedding suggestions about
function names or other details of the API :-)

Yes, the queue_*() methods should probably later make a preliminary
check of the reference's old value and return an error if the expected
value is already incorrect.  This would allow callers to fail fast if
the transaction is doomed to failure.  But that wasn't needed yet for
the one existing caller, which builds up a transaction and commits it
immediately, so I didn't implement it yet.  And the early checks would
add overhead for this caller, so maybe they should be optional anyway.
Maybe these functions should already be declared to return an error
status, but there should be an option passed to create_ref_transaction()
that selects whether fast checks should be performed or not for that
transaction.

Really, all that this first patch series does is put a different API
around the mechanism that was already there, in update_refs().  There
will be a lot more steps before we see anything approaching real
reference transactions.  But I think your (implied) suggestion, to make
the API more reminiscent of something like database transactions, is a
good one and I will work on it.

Cheers,
Michael

[1] "Guaranteed" here is of course relative.  The commit could still
fail due to the process being killed, disk errors, etc.  But it can't
fail due to lock contention with another git process.

-- 
Michael Haggerty
[email-masked]
http://softwareswirl.blogspot.com/