Re: reference-transaction regression in 2.36.0-rc1

From: Patrick Steinhardt <ps@pks.im>
Date: May 2, 2022, 11:12 UTC
Message-ID: <Ym+8iCCy5TlfmObq@ncase>
In-Reply-To: <CAJDSCnM767fdo6qy085jc9sqezvbBqDD4ikXz1y5tHEjSYED2A@mail.gmail.com>

On Mon, May 02, 2022 at 11:41:26AM +0200, Michael Heemskerk wrote:

Show 21 quoted lines

> > > I'd be happy to provide the fix to files_delete_refs in a patch
> > (including
> > > some extra tests) on top of Patrick's
> > > avoid-unnecessary-hook-invocation-with-packed-refs series if and
> > > when that series is unreverted on 'next'.
> >
> > That would be great. To be clear, do the fixes you have also fix the
> > pre-v2.36.0 issues Bryan has mentioned?
> >
> 
> That is correct. The pre-v2.36.0 issues are caused by files_delete_refs
> first
> preparing and committing a transaction for the packed_ref_store, and then
> iterating over each of the to-be-deleted refs and preparing and committing
> another transaction per ref. This latter transaction triggers the usual
> ABORTED (from packed_ref_store), PREPARED (ref_store),
> COMMITTED (ref_store) callbacks.
> 
> As far as I can tell, all we need to do is remove the separate transaction
> for
> the packed_ref_store.

Do you mean that we should just get rid of the transaction and instead delete the refs in the packet-refs backend one by one? If so, then I think that would be a performance regressions in a lot of places. If you are deleting a bunch of references at the same time, then you do indeed want to make this a single packed-refs transaction or otherwise we'd repeatedly rewrite the whole file. And given that this file can easily range in the hundreds of megabytes this can easily go into pathological cases.

I think what we need to do instead is a bit more complicated:

    1. Lock the packed-refs backend. This is to ensure that it's not
       being updated while we update loose refs.

    2. Create a transaction for the loose-refs and queue all refs that
       we are about to delete. Now we're safe to prepate the loose refs,
       and at this point in time nothing has been pruned yet. As a
       result, it is still possible for the reference-transaction hook
       to intervene even if the refs only existed as packed-refs file.

    3. Now we have to prepare and commit the packed-refs file. This is
       to avoid a race where deleting loose refs would uncover anything
       that existed in the packed-refs file already.

    4. Unlock the packed-refs backend.

    5. Commit the loose-refs transaction we have prepared already.

This should be race-free and fix the usecase for aborting ref deletions via the reference-transaction hook. It has the downside though that packed-refs are locked for far longer than they had been before because the lock also spans over preparation of the loose-refs transaction.

> Another thing we need to decide is whether or not to backport the fix to
> 2.36 and possibly older. My suggestion would be to only apply the fix to
> 'next' and NOT backport it to older git versions as changing the
> reference-transaction semantics in a patch release would be unexpected.

The way it sounds like this has been a longer-standing issue. Furthermore, it's not a regression: it just hasn't ever been working. So I don't feel like it's critical to backport this.

Show 10 quoted lines

> > Please let me know whether you want to pursue this and whether you need
> > any further help with it.
> >
> 
> I'm happy to provide the patch, but assume that I need to wait for Patrick's
> series to be reapplied to 'next'? Alternatively, I can send the patch to
> Patrick
> to be included in a reroll of the series.
> 
> Michael

Given that you said that my patch series made the preexisting problem worse I'd prefer to first land a proper fix for this issue, and only then reapply my own patches on top. I'm also happy to fix the issue myself -- at GitLab, we also care about this working correctly. Just let me know your preference.

Patrick