[RFC] fetch: update refs in a single transaction

[RFC] fetch: update refs in a single transaction

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6798 bytes --]

When git-fetch(1) isn't called with the `--atomic` flag, then each
reference will be updated in a separate transaction. As a result, even
if we failed to update a subset of transactions, the remaining refs will
be modified after the command has finished. While this pattern is
reasonably efficient with the files backend where we'd lock and write
each file separately anyway, the upcoming reftable backend could handle
such an update a lot more efficiently if it was batched into a single
transaction given that it could then create a single new reftable slice
instead of creating one slice per updated ref. While this inefficiency
can be easily mitigated by using the `--atomic` flag, this flag cannot
be used in contexts where we want partial-update semantics.

Convert git-fetch(1) to always use a single reference transaction,
regardless of whether it is called with `--atomic` or not. The only
remaining difference between both modes is that with `--atomic` set,
we'd abort the transaciton in case at least one reference cannot be
updated.

Note that this slightly changes semantics of git-fetch(1): if we hit any
unexpected errors like the reference update racing with another process,
then we'll now fail to update any references, too.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/fetch.c | 78 ++++++++++++++++---------------------------------
 1 file changed, 25 insertions(+), 53 deletions(-)

Hi,

until now, we have been quite lenient with creating lots of reference
transactions even though they could've been bundled together into a
single transaction. After all, it didn't have much of a downside in most
contexts with the files backend: we'd have to lock each loose ref
separately anyway. I'd like to get some feedback on changing our stance
here, due to multiple reasons:

    - The upcoming reftable backend will be more efficient if we use a
      single transaction to bundle together multiple ref updates given
      that it only needs to write one new slice instead of one per
      update.

    - Syncing refs to disk can be batched more efficiently if we bundle
      ref updates. See my initial patch series to implement fsync-ing
      refs [1] and Peff's benchmarks [2] demonstrating that fetches may
      become a lot slower.

    - The reference-transaction hook can be used more efficiently given
      that it would only need to execute twice, instead of twice per
      updated ref. It also has a more global view of what's happening.
      While this is a concern of mine, it's a non-reason in the context
      of the Git project given that we really ought not to change
      semantics only to appease this hook.

With these reasons in mind, I'm wondering whether we want to accept
refactorings which convert existing code to use batched reference
transactions. While the potential performance improvements are a rather
obvious upside, the downside is that it would definitely change the
failure mode in many cases.

The example I have here with git-fetch(1) would mean that if we were to
race with any other process or if any other unexpected error occurs
which leads us to die, then we'll not commit any change to disk. This
can be seen as an improvement in consistency, but it can also be seen as
a change which breaks current semantics of trying to do as much work as
possible.

I'd thus love to hear about any opinions on this topic.

Patrick

[1]: <cover.1636544377.git.ps@pks.im>
[2]: <YYwvVy6AWDjkWazn@coredump.intra.peff.net>

diff --git a/builtin/fetch.c b/builtin/fetch.c
index f7abbc31ff..c4cfd55452 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -641,9 +641,6 @@ static struct ref *get_ref_map(struct remote *remote,
 	return ref_map;
 }
 
-#define STORE_REF_ERROR_OTHER 1
-#define STORE_REF_ERROR_DF_CONFLICT 2
-
 static int s_update_ref(const char *action,
 			struct ref *ref,
 			struct ref_transaction *transaction,
@@ -651,7 +648,6 @@ static int s_update_ref(const char *action,
 {
 	char *msg;
 	char *rla = getenv("GIT_REFLOG_ACTION");
-	struct ref_transaction *our_transaction = NULL;
 	struct strbuf err = STRBUF_INIT;
 	int ret;
 
@@ -661,44 +657,12 @@ static int s_update_ref(const char *action,
 		rla = default_rla.buf;
 	msg = xstrfmt("%s: %s", rla, action);
 
-	/*
-	 * If no transaction was passed to us, we manage the transaction
-	 * ourselves. Otherwise, we trust the caller to handle the transaction
-	 * lifecycle.
-	 */
-	if (!transaction) {
-		transaction = our_transaction = ref_transaction_begin(&err);
-		if (!transaction) {
-			ret = STORE_REF_ERROR_OTHER;
-			goto out;
-		}
-	}
-
 	ret = ref_transaction_update(transaction, ref->name, &ref->new_oid,
 				     check_old ? &ref->old_oid : NULL,
 				     0, msg, &err);
-	if (ret) {
-		ret = STORE_REF_ERROR_OTHER;
-		goto out;
-	}
-
-	if (our_transaction) {
-		switch (ref_transaction_commit(our_transaction, &err)) {
-		case 0:
-			break;
-		case TRANSACTION_NAME_CONFLICT:
-			ret = STORE_REF_ERROR_DF_CONFLICT;
-			goto out;
-		default:
-			ret = STORE_REF_ERROR_OTHER;
-			goto out;
-		}
-	}
-
-out:
-	ref_transaction_free(our_transaction);
 	if (ret)
 		error("%s", err.buf);
+
 	strbuf_release(&err);
 	free(msg);
 	return ret;
@@ -1107,12 +1071,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (atomic_fetch) {
-		transaction = ref_transaction_begin(&err);
-		if (!transaction) {
-			error("%s", err.buf);
-			goto abort;
-		}
+	transaction = ref_transaction_begin(&err);
+	if (!transaction) {
+		error("%s", err.buf);
+		goto abort;
 	}
 
 	prepare_format_display(ref_map);
@@ -1229,21 +1191,31 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		}
 	}
 
-	if (!rc && transaction) {
-		rc = ref_transaction_commit(transaction, &err);
-		if (rc) {
-			error("%s", err.buf);
-			goto abort;
-		}
+	if (rc && atomic_fetch) {
+		error(_("aborting reference updates because of atomic fetch"));
+		goto abort;
 	}
 
-	if (!rc)
-		commit_fetch_head(&fetch_head);
-
-	if (rc & STORE_REF_ERROR_DF_CONFLICT)
+	switch (ref_transaction_commit(transaction, &err)) {
+	case 0:
+		break;
+	case TRANSACTION_NAME_CONFLICT:
 		error(_("some local refs could not be updated; try running\n"
 		      " 'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
+		rc = -1;
+		break;
+	default:
+		error("%s", err.buf);
+		rc = -1;
+		break;
+	}
+
+	if (rc && atomic_fetch)
+		goto abort;
+
+	if (!rc)
+		commit_fetch_head(&fetch_head);
 
 	if (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {
 		if (!fetch_show_forced_updates) {
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] fetch: update refs in a single transaction

[Christian Couder]

On Tue, Dec 7, 2021 at 11:11 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> When git-fetch(1) isn't called with the `--atomic` flag, then each
> reference will be updated in a separate transaction. As a result, even
> if we failed to update a subset of transactions, the remaining refs will
> be modified after the command has finished. While this pattern is
> reasonably efficient with the files backend where we'd lock and write
> each file separately anyway, the upcoming reftable backend could handle
> such an update a lot more efficiently if it was batched into a single
> transaction given that it could then create a single new reftable slice
> instead of creating one slice per updated ref. While this inefficiency
> can be easily mitigated by using the `--atomic` flag, this flag cannot
> be used in contexts where we want partial-update semantics.

So it seems to me that there are 3 possible behaviors/modes:

  - "atomic": a single transaction with all or nothing semantics,
currently implemented with --atomic
  - "partial-update": a single transaction with partial-update
semantics, the new behavior this patch is implementing
  - "non-atomic": many transactions (one per ref?), currently the
default when --atomic isn't passed

Yeah, "partial-update" seems better than "non-atomic" when the
reftable backend is used. But what happens  when "partial-update" is
used with the files backend?

> Convert git-fetch(1) to always use a single reference transaction,
> regardless of whether it is called with `--atomic` or not. The only
> remaining difference between both modes is that with `--atomic` set,
> we'd abort the transaciton in case at least one reference cannot be
> updated.

Nit: I would say "as soon as one reference cannot be updated"

> Note that this slightly changes semantics of git-fetch(1): if we hit any
> unexpected errors like the reference update racing with another process,
> then we'll now fail to update any references, too.

So that's one difference between the "partial-update" and the
"non-atomic" modes, but what you say doesn't really make me confident
that it's the only one.

Also are there people who are in cases where a lot of reference
updates are racing, and where it's important that in such cases at
least some ref updates succeed?

If yes, then maybe the 3 modes could be kept and options for
"partial-update" and "non-atomic" could be added. "partial-update"
could be the default in case the reftable backend is used, while
"non-atomic" would still be the default when the files backend is
used.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fetch.c | 78 ++++++++++++++++---------------------------------
>  1 file changed, 25 insertions(+), 53 deletions(-)
>
> Hi,
>
> until now, we have been quite lenient with creating lots of reference
> transactions even though they could've been bundled together into a
> single transaction. After all, it didn't have much of a downside in most
> contexts with the files backend: we'd have to lock each loose ref
> separately anyway. I'd like to get some feedback on changing our stance
> here, due to multiple reasons:
>
>     - The upcoming reftable backend will be more efficient if we use a
>       single transaction to bundle together multiple ref updates given
>       that it only needs to write one new slice instead of one per
>       update.
>
>     - Syncing refs to disk can be batched more efficiently if we bundle
>       ref updates. See my initial patch series to implement fsync-ing
>       refs [1] and Peff's benchmarks [2] demonstrating that fetches may
>       become a lot slower.

Maybe it's explained in the pointers, but is this in the case of the
files backend or the reftable backend or both?

>     - The reference-transaction hook can be used more efficiently given
>       that it would only need to execute twice, instead of twice per
>       updated ref.

Yeah, so that could be a regression for people who expect it to run
twice per updated ref.

>       It also has a more global view of what's happening.
>       While this is a concern of mine, it's a non-reason in the context
>       of the Git project given that we really ought not to change
>       semantics only to appease this hook.

Not sure I understand what you are saying here.

> With these reasons in mind, I'm wondering whether we want to accept
> refactorings which convert existing code to use batched reference
> transactions. While the potential performance improvements are a rather
> obvious upside, the downside is that it would definitely change the
> failure mode in many cases.

Another downside is that it changes how and when reference-transaction
hooks are called.

> The example I have here with git-fetch(1) would mean that if we were to
> race with any other process or if any other unexpected error occurs
> which leads us to die, then we'll not commit any change to disk. This
> can be seen as an improvement in consistency, but it can also be seen as
> a change which breaks current semantics of trying to do as much work as
> possible.

I like the idea, but it seems a bit safer to me to have 3 different
modes, so that people can test them in real life first for some time.
Then we might later be more confident with changing the default for
some backends. I might be worrying too much though, as very few people
probably use reference-transaction hooks.

Re: [RFC] fetch: update refs in a single transaction

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8536 bytes --]

On Wed, Dec 08, 2021 at 09:15:01AM +0100, Christian Couder wrote:
> On Tue, Dec 7, 2021 at 11:11 PM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > When git-fetch(1) isn't called with the `--atomic` flag, then each
> > reference will be updated in a separate transaction. As a result, even
> > if we failed to update a subset of transactions, the remaining refs will
> > be modified after the command has finished. While this pattern is
> > reasonably efficient with the files backend where we'd lock and write
> > each file separately anyway, the upcoming reftable backend could handle
> > such an update a lot more efficiently if it was batched into a single
> > transaction given that it could then create a single new reftable slice
> > instead of creating one slice per updated ref. While this inefficiency
> > can be easily mitigated by using the `--atomic` flag, this flag cannot
> > be used in contexts where we want partial-update semantics.
> 
> So it seems to me that there are 3 possible behaviors/modes:
> 
>   - "atomic": a single transaction with all or nothing semantics,
> currently implemented with --atomic
>   - "partial-update": a single transaction with partial-update
> semantics, the new behavior this patch is implementing
>   - "non-atomic": many transactions (one per ref?), currently the
> default when --atomic isn't passed
> 
> Yeah, "partial-update" seems better than "non-atomic" when the
> reftable backend is used. But what happens  when "partial-update" is
> used with the files backend?

Yeah, putting this in these three modes catches the gist of how it can
work. Currently, we only support "atomic" and "non-atomic", and this
patch proposes to replace "non-atomic" with "partial-update". I'd be
happy to instead implement it as a third mode, but if we can find a way
to "do the right thing" without having to introduce another option, then
that would be best. But as per your comments below and my own concerns
I'm not sure we can easily implement this without breaking existing
usecases.

> > Convert git-fetch(1) to always use a single reference transaction,
> > regardless of whether it is called with `--atomic` or not. The only
> > remaining difference between both modes is that with `--atomic` set,
> > we'd abort the transaciton in case at least one reference cannot be
> > updated.
> 
> Nit: I would say "as soon as one reference cannot be updated"
> 
> > Note that this slightly changes semantics of git-fetch(1): if we hit any
> > unexpected errors like the reference update racing with another process,
> > then we'll now fail to update any references, too.
> 
> So that's one difference between the "partial-update" and the
> "non-atomic" modes, but what you say doesn't really make me confident
> that it's the only one.

There can certainly be other edge cases which break, and I'm sure others
will be able to pinpoint exactly which these are.

> Also are there people who are in cases where a lot of reference
> updates are racing, and where it's important that in such cases at
> least some ref updates succeed?
>
> If yes, then maybe the 3 modes could be kept and options for
> "partial-update" and "non-atomic" could be added. "partial-update"
> could be the default in case the reftable backend is used, while
> "non-atomic" would still be the default when the files backend is
> used.

I bet there are.

> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  builtin/fetch.c | 78 ++++++++++++++++---------------------------------
> >  1 file changed, 25 insertions(+), 53 deletions(-)
> >
> > Hi,
> >
> > until now, we have been quite lenient with creating lots of reference
> > transactions even though they could've been bundled together into a
> > single transaction. After all, it didn't have much of a downside in most
> > contexts with the files backend: we'd have to lock each loose ref
> > separately anyway. I'd like to get some feedback on changing our stance
> > here, due to multiple reasons:
> >
> >     - The upcoming reftable backend will be more efficient if we use a
> >       single transaction to bundle together multiple ref updates given
> >       that it only needs to write one new slice instead of one per
> >       update.
> >
> >     - Syncing refs to disk can be batched more efficiently if we bundle
> >       ref updates. See my initial patch series to implement fsync-ing
> >       refs [1] and Peff's benchmarks [2] demonstrating that fetches may
> >       become a lot slower.
> 
> Maybe it's explained in the pointers, but is this in the case of the
> files backend or the reftable backend or both?

The discussion in [1] was in the case of the files backend: if we want
to guarantee that we never write corrupt refs to disk, then we must sync
written data to disk before moving refs into place. If using multiple
transactions, then we thus need to fsync(3P) in each commit. If we're
using a single transaction though, we can amortize the costs and batch
this, where we only fsync(3P) once after all references have been
prepared.

The same is effectively true for the reftable backend though: if we want
to ensure that reftable files are safe, then we'd have to sync them to
disk, as well.

> >     - The reference-transaction hook can be used more efficiently given
> >       that it would only need to execute twice, instead of twice per
> >       updated ref.
> 
> Yeah, so that could be a regression for people who expect it to run
> twice per updated ref.

The reference-transaction hook was always a mechanism which hooks into
internals. As such, how often the hook executes and in which order was
never guaranteed and is expected to change when implementation details
change. So this is not something I'd ever call a regression: the whole
intent of the hook is to be able to see what happens internally, not to
assert semantics or the order in which updates are performed for a
specific Git command. That's kind the trade-off with using a low-level
hook as this one.

> >       It also has a more global view of what's happening.
> >       While this is a concern of mine, it's a non-reason in the context
> >       of the Git project given that we really ought not to change
> >       semantics only to appease this hook.
> 
> Not sure I understand what you are saying here.

Previously you saw `2*len(refs)` executions of the hook, so you'd have
to piece together the puzzle to see what happens on a more global level
yourself. With the change of using a single transaction, you'd get only
two executions of the hook, where each execution has the complete
picture of all refs which are about to be updated.

> > With these reasons in mind, I'm wondering whether we want to accept
> > refactorings which convert existing code to use batched reference
> > transactions. While the potential performance improvements are a rather
> > obvious upside, the downside is that it would definitely change the
> > failure mode in many cases.
> 
> Another downside is that it changes how and when reference-transaction
> hooks are called.

As mentioned above, this is nothing I'd call a downside. We cannot ever
guarantee stable execution order of this hook, or otherwise we paint
ourselves into a corner.

> > The example I have here with git-fetch(1) would mean that if we were to
> > race with any other process or if any other unexpected error occurs
> > which leads us to die, then we'll not commit any change to disk. This
> > can be seen as an improvement in consistency, but it can also be seen as
> > a change which breaks current semantics of trying to do as much work as
> > possible.
> 
> I like the idea, but it seems a bit safer to me to have 3 different
> modes, so that people can test them in real life first for some time.
> Then we might later be more confident with changing the default for
> some backends. I might be worrying too much though, as very few people
> probably use reference-transaction hooks.

Fair, I'll wait a bit for more feedback and then probably post a v2 with
a third mode.

I think changing behaviour depending on which ref backend would be bad
precedent though. Ultimately, the user shouldn't notice which backend is
in use, except that it would ideally be faster if using the new reftable
backend. But slightly changing semantics of commands depending on which
backend is active is only going to lead to confusion.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] fetch: update refs in a single transaction

[Jeff King]

On Wed, Dec 08, 2021 at 09:15:01AM +0100, Christian Couder wrote:

> > Note that this slightly changes semantics of git-fetch(1): if we hit any
> > unexpected errors like the reference update racing with another process,
> > then we'll now fail to update any references, too.
> 
> So that's one difference between the "partial-update" and the
> "non-atomic" modes, but what you say doesn't really make me confident
> that it's the only one.
> 
> Also are there people who are in cases where a lot of reference
> updates are racing, and where it's important that in such cases at
> least some ref updates succeed?
> 
> If yes, then maybe the 3 modes could be kept and options for
> "partial-update" and "non-atomic" could be added. "partial-update"
> could be the default in case the reftable backend is used, while
> "non-atomic" would still be the default when the files backend is
> used.

I think these three modes are hard to explain to users, because the
failures which trigger for partial-update versus atomic depend on how
things happen to be implemented. Just coming from a user's perspective,
we might expect a breakdown like:

  - problems like non-fast-forward are logical policy issues, and we'd
    reject those without failing the whole transaction (in
    partial-update mode)

  - a system error like write() failing should be rare, and abandons the
    whole transaction (in either mode)

But there are some confusing cases:

  - if somebody else takes the lock and updates the ref at the same
    time, then that is handled in the ref transaction code. And so even
    though it's closer to a logical policy issue, the patch here would
    fail the whole transaction

  - likewise name conflicts (writing "refs/foo" when "refs/foo/bar"
    exists or vice versa) are more of a logical issue, but are also
    handled by the transaction code.

So I think we really have to teach the ref transaction code the notion
of non-atomic transactions, or we'll end up leaking out all of those
implementation details in user-facing behavior. I.e., the ref code needs
to learn not to immediately abort if it fails one lockfile, but to
optionally keep going (if the caller specified a non-atomic flag, of
course).

For the files-backend code, I think system errors would naturally fall
out in the same code. Failing to write() or rename() is not much
different than failing to get the lock in the first place. So
"partial-update" and "non-atomic" behavior would end up the same anyway,
and we do not need to expose the third mode to the user.

For the reftable backend, syscalls are inherently covering all the refs
anyway (we either commit the whole reftable update or not). So likewise
there would be no different between partial-update and non-atomic modes
(but they would both be different from the files backend).

I suspect the surgery needed for the ref-transaction code to allow
non-atomic updates would be pretty big, though. It involves checking
every error case to make sure it is safe to continue rather than
aborting (and munging data structures to mark particular refs as
"failed, don't do anything further for this one").

So I dunno. All of my analysis assumes the breakdown of user
expectations I gave above is a reasonable one. There may be others. But
it seems like the behavior created by just this patch would be very hard
to explain, and subject to change based on implementation details.

-Peff

Re: [RFC] fetch: update refs in a single transaction

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6126 bytes --]

On Wed, Dec 08, 2021 at 04:13:48PM -0500, Jeff King wrote:
> On Wed, Dec 08, 2021 at 09:15:01AM +0100, Christian Couder wrote:
> 
> > > Note that this slightly changes semantics of git-fetch(1): if we hit any
> > > unexpected errors like the reference update racing with another process,
> > > then we'll now fail to update any references, too.
> > 
> > So that's one difference between the "partial-update" and the
> > "non-atomic" modes, but what you say doesn't really make me confident
> > that it's the only one.
> > 
> > Also are there people who are in cases where a lot of reference
> > updates are racing, and where it's important that in such cases at
> > least some ref updates succeed?
> > 
> > If yes, then maybe the 3 modes could be kept and options for
> > "partial-update" and "non-atomic" could be added. "partial-update"
> > could be the default in case the reftable backend is used, while
> > "non-atomic" would still be the default when the files backend is
> > used.
> 
> I think these three modes are hard to explain to users, because the
> failures which trigger for partial-update versus atomic depend on how
> things happen to be implemented. Just coming from a user's perspective,
> we might expect a breakdown like:
> 
>   - problems like non-fast-forward are logical policy issues, and we'd
>     reject those without failing the whole transaction (in
>     partial-update mode)
> 
>   - a system error like write() failing should be rare, and abandons the
>     whole transaction (in either mode)
> 
> But there are some confusing cases:
> 
>   - if somebody else takes the lock and updates the ref at the same
>     time, then that is handled in the ref transaction code. And so even
>     though it's closer to a logical policy issue, the patch here would
>     fail the whole transaction
> 
>   - likewise name conflicts (writing "refs/foo" when "refs/foo/bar"
>     exists or vice versa) are more of a logical issue, but are also
>     handled by the transaction code.
> 
> So I think we really have to teach the ref transaction code the notion
> of non-atomic transactions, or we'll end up leaking out all of those
> implementation details in user-facing behavior. I.e., the ref code needs
> to learn not to immediately abort if it fails one lockfile, but to
> optionally keep going (if the caller specified a non-atomic flag, of
> course).

That would probably be the most flexible approach for the future, too.
There's going to be several locations which could benefit from such a
change, where we do want to get the performance benefits of using a
single transaction while continuing to exhibit the same behaviour in
edge cases where only a subset of refs could be updated.

For what it's worth, I think that such a new mode should likely only
kick in when "preparing" a transaction: this is the last step before
"committing" it and would thus be the only place where the caller has a
chance to introspect what really has been queued up and what had to be
dropped from the transaction due to races or whatnot. As long as callers
get a list of all dropped refs, including why they have been dropped,
they can also still communicate this information to the user.

> For the files-backend code, I think system errors would naturally fall
> out in the same code. Failing to write() or rename() is not much
> different than failing to get the lock in the first place. So
> "partial-update" and "non-atomic" behavior would end up the same anyway,
> and we do not need to expose the third mode to the user.

I think I disagree here. Failing to write() to me is quite different
from failing to take a lock: the first one is an unexpected system-level
error and brings us into a situation where we ain't got no clue why it
happened. The second one is a logical error that is entirely expected
given that lockfiles are explicitly designed for this failure mode, so
we know why they happen. With this in mind, I'd argue that we should
only continue with the transaction in the latter case, and abort on
unexpected system-level errors.

> For the reftable backend, syscalls are inherently covering all the refs
> anyway (we either commit the whole reftable update or not). So likewise
> there would be no different between partial-update and non-atomic modes
> (but they would both be different from the files backend).
> 
> I suspect the surgery needed for the ref-transaction code to allow
> non-atomic updates would be pretty big, though. It involves checking
> every error case to make sure it is safe to continue rather than
> aborting (and munging data structures to mark particular refs as
> "failed, don't do anything further for this one").

I hope that it's not going to be that bad if we restrict it to the
"prepare" phase, but that may just be wishful thinking.

> So I dunno. All of my analysis assumes the breakdown of user
> expectations I gave above is a reasonable one. There may be others. But
> it seems like the behavior created by just this patch would be very hard
> to explain, and subject to change based on implementation details.
> 
> -Peff

I think your analysis makes sense.

While I think that the three modes aren't as bad for a command like
git-fetch(1), the "single-tx-partial-update" mode would really only be a
special case of our current default, except that we use a single
transaction, only. Users shouldn't really need to care about this, and
we should do the right thing by default. The discussion mostly came to
live because our current implementation of reference transactions is
lacking the ability to handle this partial-update mode well (and it
didn't really have to until now), but pushing this technical limitation
onto the user is probably nothing we should do.

In the end I agree that we ought to extend the reftx mechanism to
support this usecase. While it's more work up-front, I expect that other
commands could benefit in a similar way without having to add
`--partial-atomic-reference-updates` switches to all of them.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC] fetch: update refs in a single transaction

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> instead of creating one slice per updated ref. While this inefficiency
> can be easily mitigated by using the `--atomic` flag, this flag cannot
> be used in contexts where we want partial-update semantics.

Interesting and puzzling.  In today's code, we use a single
transaction when "atomic" is asked for, so that we can open a
transaction, prepare bunch of ref updates, and say "commit" to
commit all of them and let the ref_transaction layer to make the
whole thing all-or-none.  If we now use a single transaction for two
refs update that do not have to be atomic, it is surprising (from
the diffstat) that we can do so without changing anything in the
ref_transaction layer.  Doesn't the caller at least need to say
"this transaction is best-effort 'partial-update' (whatever it
means)" vs "this transaction is all-or-none"?  And doesn't the
ref_transaction layer now need to implement the 'partial-update'
thing?

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/fetch.c | 78 ++++++++++++++++---------------------------------
>  1 file changed, 25 insertions(+), 53 deletions(-)

Re: [RFC] fetch: update refs in a single transaction

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I suspect the surgery needed for the ref-transaction code to allow
> non-atomic updates would be pretty big, though. It involves checking
> every error case to make sure it is safe to continue rather than
> aborting (and munging data structures to mark particular refs as
> "failed, don't do anything further for this one").
>
> So I dunno. All of my analysis assumes the breakdown of user
> expectations I gave above is a reasonable one. There may be others. But
> it seems like the behavior created by just this patch would be very hard
> to explain, and subject to change based on implementation details.

Oh, I should have read your analysis before reacting to the original
message myself ;-)

Yes, current callers of ref-transaction code may have some logic to
decide that it is not even worth telling a proposed change to the
ref API (e.g. non-fast-forward) but that does feel like an
unnecessary implementation detail, and a true "partial transaction"
needs cooperation by the ref-transaction layer.

And when it is done, we do not have to explain anything to the user
beyond what we already do.  The "--atomic" option will make it
all-or-none, and without it, changes to each ref may or may not fail
individually with its own reason, without correlation to the outcome
of the changes to any other refs.  And use of single or multiple
transactions just becomes an implementation detail of non-atomic
updates.

Re: [RFC] fetch: update refs in a single transaction

[Jeff King]

On Thu, Dec 09, 2021 at 08:11:34AM +0100, Patrick Steinhardt wrote:

> > For the files-backend code, I think system errors would naturally fall
> > out in the same code. Failing to write() or rename() is not much
> > different than failing to get the lock in the first place. So
> > "partial-update" and "non-atomic" behavior would end up the same anyway,
> > and we do not need to expose the third mode to the user.
> 
> I think I disagree here. Failing to write() to me is quite different
> from failing to take a lock: the first one is an unexpected system-level
> error and brings us into a situation where we ain't got no clue why it
> happened. The second one is a logical error that is entirely expected
> given that lockfiles are explicitly designed for this failure mode, so
> we know why they happen. With this in mind, I'd argue that we should
> only continue with the transaction in the latter case, and abort on
> unexpected system-level errors.

Just to be clear, it's not that I necessarily think these error types
are logically related. I only meant that once you are detecting and
recovering from one type, it would be easy to implement it either way.

I'd be OK with either type of behavior. If that was the only difference
between partial-update and non-atomic, though, I'm not sure if that
merits exposing the complexity of a "third mode" to the user. But I
don't feel strongly about it either way.

> > I suspect the surgery needed for the ref-transaction code to allow
> > non-atomic updates would be pretty big, though. It involves checking
> > every error case to make sure it is safe to continue rather than
> > aborting (and munging data structures to mark particular refs as
> > "failed, don't do anything further for this one").
> 
> I hope that it's not going to be that bad if we restrict it to the
> "prepare" phase, but that may just be wishful thinking.

Yeah, maybe. :) I didn't look closely, so it may not be too bad. I just
remember the refs system being very finicky about things like failure,
races, etc. But I'm sure you'll figure it out once you start looking
closely. :)

One thing to watch out for is that in the files backend, _part_ of the
update may be shared by multiple refs: updating the packed-refs file (if
we are deleting refs). So if you are deleting "refs/heads/foo" and
"refs/heads/bar", but taking the lock on "foo" fails, you'd want to make
sure only to delete "bar" from packed-refs, not "foo".

-Peff