Re: [PATCH v4 0/3] Make update refs more atomic

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Apr 16, 2014, 21:31 UTC
Message-ID: <CAL=YDWnHtPedxYmpycgSybZA=CmdD55XQAFdA-Bs_42bk2Z0Tg@mail.gmail.com>
In-Reply-To: <xmqq1twxgjge.fsf@gitster.dls.corp.google.com>

On Wed, Apr 16, 2014 at 12:31 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 28 quoted lines

> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> Currently any locking of refs in a transaction only happens during the commit
>> phase. I think it would be useful to have a mechanism where you could
>> optionally take out locks for the involved refs early during the transaction.
>> So that simple callers could continue using
>> ref_transaction_begin()
>> ref_transaction_create|update|delete()*
>> ref_transaction_commit()
>>
>> but, if a caller such as walker_fetch() could opt to do
>> ref_transaction_begin()
>> ref_transaction_lock_ref()*
>> ...do stuff...
>> ref_transaction_create|update|delete()*
>> ref_transaction_commit()
>>
>> In this second case ref_transaction_commit() would only take out any locks that
>> are missing during the 'lock the refs" loop.
>>
>> Suggestion 1: Add a ref_transaction_lock_ref() to allow locking a ref
>> early during
>> a transaction.
>
> Hmph.
>
> I am not sure if that is the right way to go, or instead change all
> create/update/delete to take locks without adding a new primitive.

ack.

Show 28 quoted lines

>
>> A second idea is to change the signatures for
>> ref_transaction_create|update|delete()
>> slightly and allow them to return errors early.
>> We can check for things like add_update() failing, check that the
>> ref-name looks sane,
>> check some of the flags, like if has_old==true then old sha1 should
>> not be NULL or 0{40}, etc.
>>
>> Additionally for robustness, if any of these functions detect an error
>> we can flag this in the
>> transaction structure and take action during ref_transaction_commit().
>> I.e. if a ref_transaction_update had a hard failure, do not allow
>> ref_transaction_commit()
>> to succeed.
>>
>> Suggestion 2: Change ref_transaction_create|delete|update() to return an int.
>> All callers that use these functions should check the function for error.
>
> I think that is a very sensible thing to do.
>
> The details of determining "this cannot possibly succeed" may change
> (for example, if we have them take locks at the point of
> create/delete/update, a failure to lock may count as an early
> error).
>
> Is there any reason why this should be conditional (i.e. you said
> "allow them to", implying that the early failure is optional)?

It was poor wording on my side. Checking for the ref_transaction_*() return for error should be mandatory (modulo bugs).

But a caller could be buggy and fail to check properly. It would be very cheap to detect this condition in ref_transaction_commit() which could then do

  die("transaction commit called for errored transaction");

which would make it easy to spot this kind of bugs.

Show 14 quoted lines

>
>> Suggestion 3: remove the qsort and check for duplicates in
>> ref_transaction_commit()
>> Since we are already taking out a lock for each ref we are updating
>> during the transaction
>> any duplicate refs will fail the second attempt to lock the same ref which will
>> implicitly make sure that a transaction will not change the same ref twice.
>
> I do not know if I care about the implementation detail of "do we
> have a unique set of update requests?".  While I do not see a strong
> need for one transaction to touch the same ref twice (e.g. create to
> point at commit A and update it to point at commit B), I do not see
> why we should forbid such a use in the future.
>

ack.