{"thread":{"id":"57046","subject":"[RFC] fetch: update refs in a single transaction","startedAt":"2021-12-07T14:25:06Z","lastAt":"2021-12-10T09:04:02Z","messageCount":8,"participants":["Patrick Steinhardt","Christian Couder","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"443284","messageId":"259de62b26302c10f429d52bec42a8a954bc5ba3.1638886972.git.ps@pks.im","threadId":"57046","inReplyTo":null,"subject":"[RFC] fetch: update refs in a single transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-07T14:24:21Z","receivedAt":"2021-12-07T14:25:06Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When git-fetch(1) isn't called with the `--atomic` flag, then each\nreference will be updated in a separate transaction. As a result, even\nif we failed to update a subset of transactions, the remaining refs will\nbe modified after the command has finished. While this pattern is\nreasonably efficient with the files backend where we'd lock and write\neach file separately anyway, the upcoming reftable backend could handle\nsuch an update a lot more efficiently if it was batched into a single\ntransaction given that it could then create a single new reftable slice\ninstead of creating one slice per updated ref. While this inefficiency\ncan be easily mitigated by using the `--atomic` flag, this flag cannot\nbe used in contexts where we want partial-update semantics.\n\nConvert git-fetch(1) to always use a single reference transaction,\nregardless of whether it is called with `--atomic` or not. The only\nremaining difference between both modes is that with `--atomic` set,\nwe'd abort the transaciton in case at least one reference cannot be\nupdated.\n\nNote that this slightly changes semantics of git-fetch(1): if we hit any\nunexpected errors like the reference update racing with another process,\nthen we'll now fail to update any references, too.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/fetch.c | 78 ++++++++++++++++---------------------------------\n 1 file changed, 25 insertions(+), 53 deletions(-)\n\nHi,\n\nuntil now, we have been quite lenient with creating lots of reference\ntransactions even though they could've been bundled together into a\nsingle transaction. After all, it didn't have much of a downside in most\ncontexts with the files backend: we'd have to lock each loose ref\nseparately anyway. I'd like to get some feedback on changing our stance\nhere, due to multiple reasons:\n\n    - The upcoming reftable backend will be more efficient if we use a\n      single transaction to bundle together multiple ref updates given\n      that it only needs to write one new slice instead of one per\n      update.\n\n    - Syncing refs to disk can be batched more efficiently if we bundle\n      ref updates. See my initial patch series to implement fsync-ing\n      refs [1] and Peff's benchmarks [2] demonstrating that fetches may\n      become a lot slower.\n\n    - The reference-transaction hook can be used more efficiently given\n      that it would only need to execute twice, instead of twice per\n      updated ref. It also has a more global view of what's happening.\n      While this is a concern of mine, it's a non-reason in the context\n      of the Git project given that we really ought not to change\n      semantics only to appease this hook.\n\nWith these reasons in mind, I'm wondering whether we want to accept\nrefactorings which convert existing code to use batched reference\ntransactions. While the potential performance improvements are a rather\nobvious upside, the downside is that it would definitely change the\nfailure mode in many cases.\n\nThe example I have here with git-fetch(1) would mean that if we were to\nrace with any other process or if any other unexpected error occurs\nwhich leads us to die, then we'll not commit any change to disk. This\ncan be seen as an improvement in consistency, but it can also be seen as\na change which breaks current semantics of trying to do as much work as\npossible.\n\nI'd thus love to hear about any opinions on this topic.\n\nPatrick\n\n[1]: <cover.1636544377.git.ps@pks.im>\n[2]: <YYwvVy6AWDjkWazn@coredump.intra.peff.net>\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex f7abbc31ff..c4cfd55452 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -641,9 +641,6 @@ static struct ref *get_ref_map(struct remote *remote,\n \treturn ref_map;\n }\n \n-#define STORE_REF_ERROR_OTHER 1\n-#define STORE_REF_ERROR_DF_CONFLICT 2\n-\n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n \t\t\tstruct ref_transaction *transaction,\n@@ -651,7 +648,6 @@ static int s_update_ref(const char *action,\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n@@ -661,44 +657,12 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\t/*\n-\t * If no transaction was passed to us, we manage the transaction\n-\t * ourselves. Otherwise, we trust the caller to handle the transaction\n-\t * lifecycle.\n-\t */\n-\tif (!transaction) {\n-\t\ttransaction = our_transaction = ref_transaction_begin(&err);\n-\t\tif (!transaction) {\n-\t\t\tret = STORE_REF_ERROR_OTHER;\n-\t\t\tgoto out;\n-\t\t}\n-\t}\n-\n \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n \t\t\t\t     check_old ? &ref->old_oid : NULL,\n \t\t\t\t     0, msg, &err);\n-\tif (ret) {\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n-\t}\n-\n-\tif (our_transaction) {\n-\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n-\t\tcase 0:\n-\t\t\tbreak;\n-\t\tcase TRANSACTION_NAME_CONFLICT:\n-\t\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n-\t\t\tgoto out;\n-\t\tdefault:\n-\t\t\tret = STORE_REF_ERROR_OTHER;\n-\t\t\tgoto out;\n-\t\t}\n-\t}\n-\n-out:\n-\tref_transaction_free(our_transaction);\n \tif (ret)\n \t\terror(\"%s\", err.buf);\n+\n \tstrbuf_release(&err);\n \tfree(msg);\n \treturn ret;\n@@ -1107,12 +1071,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n-\tif (atomic_fetch) {\n-\t\ttransaction = ref_transaction_begin(&err);\n-\t\tif (!transaction) {\n-\t\t\terror(\"%s\", err.buf);\n-\t\t\tgoto abort;\n-\t\t}\n+\ttransaction = ref_transaction_begin(&err);\n+\tif (!transaction) {\n+\t\terror(\"%s\", err.buf);\n+\t\tgoto abort;\n \t}\n \n \tprepare_format_display(ref_map);\n@@ -1229,21 +1191,31 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\t}\n \t}\n \n-\tif (!rc && transaction) {\n-\t\trc = ref_transaction_commit(transaction, &err);\n-\t\tif (rc) {\n-\t\t\terror(\"%s\", err.buf);\n-\t\t\tgoto abort;\n-\t\t}\n+\tif (rc && atomic_fetch) {\n+\t\terror(_(\"aborting reference updates because of atomic fetch\"));\n+\t\tgoto abort;\n \t}\n \n-\tif (!rc)\n-\t\tcommit_fetch_head(&fetch_head);\n-\n-\tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n+\tswitch (ref_transaction_commit(transaction, &err)) {\n+\tcase 0:\n+\t\tbreak;\n+\tcase TRANSACTION_NAME_CONFLICT:\n \t\terror(_(\"some local refs could not be updated; try running\\n\"\n \t\t      \" 'git remote prune %s' to remove any old, conflicting \"\n \t\t      \"branches\"), remote_name);\n+\t\trc = -1;\n+\t\tbreak;\n+\tdefault:\n+\t\terror(\"%s\", err.buf);\n+\t\trc = -1;\n+\t\tbreak;\n+\t}\n+\n+\tif (rc && atomic_fetch)\n+\t\tgoto abort;\n+\n+\tif (!rc)\n+\t\tcommit_fetch_head(&fetch_head);\n \n \tif (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {\n \t\tif (!fetch_show_forced_updates) {\n-- \n2.34.1\n\n"},{"id":"443410","messageId":"CAP8UFD0Y-2aD6ywRFi49_emxzcLqrfyNpZ29fgsJ0FKc0njYqg@mail.gmail.com","threadId":"57046","inReplyTo":"259de62b26302c10f429d52bec42a8a954bc5ba3.1638886972.git.ps@pks.im","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-12-08T08:15:01Z","receivedAt":"2021-12-08T08:15:15Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Tue, Dec 7, 2021 at 11:11 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> When git-fetch(1) isn't called with the `--atomic` flag, then each\n> reference will be updated in a separate transaction. As a result, even\n> if we failed to update a subset of transactions, the remaining refs will\n> be modified after the command has finished. While this pattern is\n> reasonably efficient with the files backend where we'd lock and write\n> each file separately anyway, the upcoming reftable backend could handle\n> such an update a lot more efficiently if it was batched into a single\n> transaction given that it could then create a single new reftable slice\n> instead of creating one slice per updated ref. While this inefficiency\n> can be easily mitigated by using the `--atomic` flag, this flag cannot\n> be used in contexts where we want partial-update semantics.\n\nSo it seems to me that there are 3 possible behaviors/modes:\n\n  - \"atomic\": a single transaction with all or nothing semantics,\ncurrently implemented with --atomic\n  - \"partial-update\": a single transaction with partial-update\nsemantics, the new behavior this patch is implementing\n  - \"non-atomic\": many transactions (one per ref?), currently the\ndefault when --atomic isn't passed\n\nYeah, \"partial-update\" seems better than \"non-atomic\" when the\nreftable backend is used. But what happens  when \"partial-update\" is\nused with the files backend?\n\n> Convert git-fetch(1) to always use a single reference transaction,\n> regardless of whether it is called with `--atomic` or not. The only\n> remaining difference between both modes is that with `--atomic` set,\n> we'd abort the transaciton in case at least one reference cannot be\n> updated.\n\nNit: I would say \"as soon as one reference cannot be updated\"\n\n> Note that this slightly changes semantics of git-fetch(1): if we hit any\n> unexpected errors like the reference update racing with another process,\n> then we'll now fail to update any references, too.\n\nSo that's one difference between the \"partial-update\" and the\n\"non-atomic\" modes, but what you say doesn't really make me confident\nthat it's the only one.\n\nAlso are there people who are in cases where a lot of reference\nupdates are racing, and where it's important that in such cases at\nleast some ref updates succeed?\n\nIf yes, then maybe the 3 modes could be kept and options for\n\"partial-update\" and \"non-atomic\" could be added. \"partial-update\"\ncould be the default in case the reftable backend is used, while\n\"non-atomic\" would still be the default when the files backend is\nused.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/fetch.c | 78 ++++++++++++++++---------------------------------\n>  1 file changed, 25 insertions(+), 53 deletions(-)\n>\n> Hi,\n>\n> until now, we have been quite lenient with creating lots of reference\n> transactions even though they could've been bundled together into a\n> single transaction. After all, it didn't have much of a downside in most\n> contexts with the files backend: we'd have to lock each loose ref\n> separately anyway. I'd like to get some feedback on changing our stance\n> here, due to multiple reasons:\n>\n>     - The upcoming reftable backend will be more efficient if we use a\n>       single transaction to bundle together multiple ref updates given\n>       that it only needs to write one new slice instead of one per\n>       update.\n>\n>     - Syncing refs to disk can be batched more efficiently if we bundle\n>       ref updates. See my initial patch series to implement fsync-ing\n>       refs [1] and Peff's benchmarks [2] demonstrating that fetches may\n>       become a lot slower.\n\nMaybe it's explained in the pointers, but is this in the case of the\nfiles backend or the reftable backend or both?\n\n>     - The reference-transaction hook can be used more efficiently given\n>       that it would only need to execute twice, instead of twice per\n>       updated ref.\n\nYeah, so that could be a regression for people who expect it to run\ntwice per updated ref.\n\n>       It also has a more global view of what's happening.\n>       While this is a concern of mine, it's a non-reason in the context\n>       of the Git project given that we really ought not to change\n>       semantics only to appease this hook.\n\nNot sure I understand what you are saying here.\n\n> With these reasons in mind, I'm wondering whether we want to accept\n> refactorings which convert existing code to use batched reference\n> transactions. While the potential performance improvements are a rather\n> obvious upside, the downside is that it would definitely change the\n> failure mode in many cases.\n\nAnother downside is that it changes how and when reference-transaction\nhooks are called.\n\n> The example I have here with git-fetch(1) would mean that if we were to\n> race with any other process or if any other unexpected error occurs\n> which leads us to die, then we'll not commit any change to disk. This\n> can be seen as an improvement in consistency, but it can also be seen as\n> a change which breaks current semantics of trying to do as much work as\n> possible.\n\nI like the idea, but it seems a bit safer to me to have 3 different\nmodes, so that people can test them in real life first for some time.\nThen we might later be more confident with changing the default for\nsome backends. I might be worrying too much though, as very few people\nprobably use reference-transaction hooks.\n"},{"id":"443411","messageId":"YbBxT+s8SssdkH7Z@ncase","threadId":"57046","inReplyTo":"CAP8UFD0Y-2aD6ywRFi49_emxzcLqrfyNpZ29fgsJ0FKc0njYqg@mail.gmail.com","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-08T08:48:15Z","receivedAt":"2021-12-08T08:49:05Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 08, 2021 at 09:15:01AM +0100, Christian Couder wrote:\n> On Tue, Dec 7, 2021 at 11:11 PM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > When git-fetch(1) isn't called with the `--atomic` flag, then each\n> > reference will be updated in a separate transaction. As a result, even\n> > if we failed to update a subset of transactions, the remaining refs will\n> > be modified after the command has finished. While this pattern is\n> > reasonably efficient with the files backend where we'd lock and write\n> > each file separately anyway, the upcoming reftable backend could handle\n> > such an update a lot more efficiently if it was batched into a single\n> > transaction given that it could then create a single new reftable slice\n> > instead of creating one slice per updated ref. While this inefficiency\n> > can be easily mitigated by using the `--atomic` flag, this flag cannot\n> > be used in contexts where we want partial-update semantics.\n> \n> So it seems to me that there are 3 possible behaviors/modes:\n> \n>   - \"atomic\": a single transaction with all or nothing semantics,\n> currently implemented with --atomic\n>   - \"partial-update\": a single transaction with partial-update\n> semantics, the new behavior this patch is implementing\n>   - \"non-atomic\": many transactions (one per ref?), currently the\n> default when --atomic isn't passed\n> \n> Yeah, \"partial-update\" seems better than \"non-atomic\" when the\n> reftable backend is used. But what happens  when \"partial-update\" is\n> used with the files backend?\n\nYeah, putting this in these three modes catches the gist of how it can\nwork. Currently, we only support \"atomic\" and \"non-atomic\", and this\npatch proposes to replace \"non-atomic\" with \"partial-update\". I'd be\nhappy to instead implement it as a third mode, but if we can find a way\nto \"do the right thing\" without having to introduce another option, then\nthat would be best. But as per your comments below and my own concerns\nI'm not sure we can easily implement this without breaking existing\nusecases.\n\n> > Convert git-fetch(1) to always use a single reference transaction,\n> > regardless of whether it is called with `--atomic` or not. The only\n> > remaining difference between both modes is that with `--atomic` set,\n> > we'd abort the transaciton in case at least one reference cannot be\n> > updated.\n> \n> Nit: I would say \"as soon as one reference cannot be updated\"\n> \n> > Note that this slightly changes semantics of git-fetch(1): if we hit any\n> > unexpected errors like the reference update racing with another process,\n> > then we'll now fail to update any references, too.\n> \n> So that's one difference between the \"partial-update\" and the\n> \"non-atomic\" modes, but what you say doesn't really make me confident\n> that it's the only one.\n\nThere can certainly be other edge cases which break, and I'm sure others\nwill be able to pinpoint exactly which these are.\n\n> Also are there people who are in cases where a lot of reference\n> updates are racing, and where it's important that in such cases at\n> least some ref updates succeed?\n>\n> If yes, then maybe the 3 modes could be kept and options for\n> \"partial-update\" and \"non-atomic\" could be added. \"partial-update\"\n> could be the default in case the reftable backend is used, while\n> \"non-atomic\" would still be the default when the files backend is\n> used.\n\nI bet there are.\n\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  builtin/fetch.c | 78 ++++++++++++++++---------------------------------\n> >  1 file changed, 25 insertions(+), 53 deletions(-)\n> >\n> > Hi,\n> >\n> > until now, we have been quite lenient with creating lots of reference\n> > transactions even though they could've been bundled together into a\n> > single transaction. After all, it didn't have much of a downside in most\n> > contexts with the files backend: we'd have to lock each loose ref\n> > separately anyway. I'd like to get some feedback on changing our stance\n> > here, due to multiple reasons:\n> >\n> >     - The upcoming reftable backend will be more efficient if we use a\n> >       single transaction to bundle together multiple ref updates given\n> >       that it only needs to write one new slice instead of one per\n> >       update.\n> >\n> >     - Syncing refs to disk can be batched more efficiently if we bundle\n> >       ref updates. See my initial patch series to implement fsync-ing\n> >       refs [1] and Peff's benchmarks [2] demonstrating that fetches may\n> >       become a lot slower.\n> \n> Maybe it's explained in the pointers, but is this in the case of the\n> files backend or the reftable backend or both?\n\nThe discussion in [1] was in the case of the files backend: if we want\nto guarantee that we never write corrupt refs to disk, then we must sync\nwritten data to disk before moving refs into place. If using multiple\ntransactions, then we thus need to fsync(3P) in each commit. If we're\nusing a single transaction though, we can amortize the costs and batch\nthis, where we only fsync(3P) once after all references have been\nprepared.\n\nThe same is effectively true for the reftable backend though: if we want\nto ensure that reftable files are safe, then we'd have to sync them to\ndisk, as well.\n\n> >     - The reference-transaction hook can be used more efficiently given\n> >       that it would only need to execute twice, instead of twice per\n> >       updated ref.\n> \n> Yeah, so that could be a regression for people who expect it to run\n> twice per updated ref.\n\nThe reference-transaction hook was always a mechanism which hooks into\ninternals. As such, how often the hook executes and in which order was\nnever guaranteed and is expected to change when implementation details\nchange. So this is not something I'd ever call a regression: the whole\nintent of the hook is to be able to see what happens internally, not to\nassert semantics or the order in which updates are performed for a\nspecific Git command. That's kind the trade-off with using a low-level\nhook as this one.\n\n> >       It also has a more global view of what's happening.\n> >       While this is a concern of mine, it's a non-reason in the context\n> >       of the Git project given that we really ought not to change\n> >       semantics only to appease this hook.\n> \n> Not sure I understand what you are saying here.\n\nPreviously you saw `2*len(refs)` executions of the hook, so you'd have\nto piece together the puzzle to see what happens on a more global level\nyourself. With the change of using a single transaction, you'd get only\ntwo executions of the hook, where each execution has the complete\npicture of all refs which are about to be updated.\n\n> > With these reasons in mind, I'm wondering whether we want to accept\n> > refactorings which convert existing code to use batched reference\n> > transactions. While the potential performance improvements are a rather\n> > obvious upside, the downside is that it would definitely change the\n> > failure mode in many cases.\n> \n> Another downside is that it changes how and when reference-transaction\n> hooks are called.\n\nAs mentioned above, this is nothing I'd call a downside. We cannot ever\nguarantee stable execution order of this hook, or otherwise we paint\nourselves into a corner.\n\n> > The example I have here with git-fetch(1) would mean that if we were to\n> > race with any other process or if any other unexpected error occurs\n> > which leads us to die, then we'll not commit any change to disk. This\n> > can be seen as an improvement in consistency, but it can also be seen as\n> > a change which breaks current semantics of trying to do as much work as\n> > possible.\n> \n> I like the idea, but it seems a bit safer to me to have 3 different\n> modes, so that people can test them in real life first for some time.\n> Then we might later be more confident with changing the default for\n> some backends. I might be worrying too much though, as very few people\n> probably use reference-transaction hooks.\n\nFair, I'll wait a bit for more feedback and then probably post a v2 with\na third mode.\n\nI think changing behaviour depending on which ref backend would be bad\nprecedent though. Ultimately, the user shouldn't notice which backend is\nin use, except that it would ideally be faster if using the new reftable\nbackend. But slightly changing semantics of commands depending on which\nbackend is active is only going to lead to confusion.\n\nPatrick\n"},{"id":"443519","messageId":"YbEgDBJPuGXQ+2t6@coredump.intra.peff.net","threadId":"57046","inReplyTo":"CAP8UFD0Y-2aD6ywRFi49_emxzcLqrfyNpZ29fgsJ0FKc0njYqg@mail.gmail.com","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-08T21:13:48Z","receivedAt":"2021-12-08T21:13:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 08, 2021 at 09:15:01AM +0100, Christian Couder wrote:\n\n> > Note that this slightly changes semantics of git-fetch(1): if we hit any\n> > unexpected errors like the reference update racing with another process,\n> > then we'll now fail to update any references, too.\n> \n> So that's one difference between the \"partial-update\" and the\n> \"non-atomic\" modes, but what you say doesn't really make me confident\n> that it's the only one.\n> \n> Also are there people who are in cases where a lot of reference\n> updates are racing, and where it's important that in such cases at\n> least some ref updates succeed?\n> \n> If yes, then maybe the 3 modes could be kept and options for\n> \"partial-update\" and \"non-atomic\" could be added. \"partial-update\"\n> could be the default in case the reftable backend is used, while\n> \"non-atomic\" would still be the default when the files backend is\n> used.\n\nI think these three modes are hard to explain to users, because the\nfailures which trigger for partial-update versus atomic depend on how\nthings happen to be implemented. Just coming from a user's perspective,\nwe might expect a breakdown like:\n\n  - problems like non-fast-forward are logical policy issues, and we'd\n    reject those without failing the whole transaction (in\n    partial-update mode)\n\n  - a system error like write() failing should be rare, and abandons the\n    whole transaction (in either mode)\n\nBut there are some confusing cases:\n\n  - if somebody else takes the lock and updates the ref at the same\n    time, then that is handled in the ref transaction code. And so even\n    though it's closer to a logical policy issue, the patch here would\n    fail the whole transaction\n\n  - likewise name conflicts (writing \"refs/foo\" when \"refs/foo/bar\"\n    exists or vice versa) are more of a logical issue, but are also\n    handled by the transaction code.\n\nSo I think we really have to teach the ref transaction code the notion\nof non-atomic transactions, or we'll end up leaking out all of those\nimplementation details in user-facing behavior. I.e., the ref code needs\nto learn not to immediately abort if it fails one lockfile, but to\noptionally keep going (if the caller specified a non-atomic flag, of\ncourse).\n\nFor the files-backend code, I think system errors would naturally fall\nout in the same code. Failing to write() or rename() is not much\ndifferent than failing to get the lock in the first place. So\n\"partial-update\" and \"non-atomic\" behavior would end up the same anyway,\nand we do not need to expose the third mode to the user.\n\nFor the reftable backend, syscalls are inherently covering all the refs\nanyway (we either commit the whole reftable update or not). So likewise\nthere would be no different between partial-update and non-atomic modes\n(but they would both be different from the files backend).\n\nI suspect the surgery needed for the ref-transaction code to allow\nnon-atomic updates would be pretty big, though. It involves checking\nevery error case to make sure it is safe to continue rather than\naborting (and munging data structures to mark particular refs as\n\"failed, don't do anything further for this one\").\n\nSo I dunno. All of my analysis assumes the breakdown of user\nexpectations I gave above is a reasonable one. There may be others. But\nit seems like the behavior created by just this patch would be very hard\nto explain, and subject to change based on implementation details.\n\n-Peff\n"},{"id":"443607","messageId":"YbGsJnTQov6byXzF@ncase","threadId":"57046","inReplyTo":"YbEgDBJPuGXQ+2t6@coredump.intra.peff.net","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2021-12-09T07:11:34Z","receivedAt":"2021-12-09T07:12:21Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Dec 08, 2021 at 04:13:48PM -0500, Jeff King wrote:\n> On Wed, Dec 08, 2021 at 09:15:01AM +0100, Christian Couder wrote:\n> \n> > > Note that this slightly changes semantics of git-fetch(1): if we hit any\n> > > unexpected errors like the reference update racing with another process,\n> > > then we'll now fail to update any references, too.\n> > \n> > So that's one difference between the \"partial-update\" and the\n> > \"non-atomic\" modes, but what you say doesn't really make me confident\n> > that it's the only one.\n> > \n> > Also are there people who are in cases where a lot of reference\n> > updates are racing, and where it's important that in such cases at\n> > least some ref updates succeed?\n> > \n> > If yes, then maybe the 3 modes could be kept and options for\n> > \"partial-update\" and \"non-atomic\" could be added. \"partial-update\"\n> > could be the default in case the reftable backend is used, while\n> > \"non-atomic\" would still be the default when the files backend is\n> > used.\n> \n> I think these three modes are hard to explain to users, because the\n> failures which trigger for partial-update versus atomic depend on how\n> things happen to be implemented. Just coming from a user's perspective,\n> we might expect a breakdown like:\n> \n>   - problems like non-fast-forward are logical policy issues, and we'd\n>     reject those without failing the whole transaction (in\n>     partial-update mode)\n> \n>   - a system error like write() failing should be rare, and abandons the\n>     whole transaction (in either mode)\n> \n> But there are some confusing cases:\n> \n>   - if somebody else takes the lock and updates the ref at the same\n>     time, then that is handled in the ref transaction code. And so even\n>     though it's closer to a logical policy issue, the patch here would\n>     fail the whole transaction\n> \n>   - likewise name conflicts (writing \"refs/foo\" when \"refs/foo/bar\"\n>     exists or vice versa) are more of a logical issue, but are also\n>     handled by the transaction code.\n> \n> So I think we really have to teach the ref transaction code the notion\n> of non-atomic transactions, or we'll end up leaking out all of those\n> implementation details in user-facing behavior. I.e., the ref code needs\n> to learn not to immediately abort if it fails one lockfile, but to\n> optionally keep going (if the caller specified a non-atomic flag, of\n> course).\n\nThat would probably be the most flexible approach for the future, too.\nThere's going to be several locations which could benefit from such a\nchange, where we do want to get the performance benefits of using a\nsingle transaction while continuing to exhibit the same behaviour in\nedge cases where only a subset of refs could be updated.\n\nFor what it's worth, I think that such a new mode should likely only\nkick in when \"preparing\" a transaction: this is the last step before\n\"committing\" it and would thus be the only place where the caller has a\nchance to introspect what really has been queued up and what had to be\ndropped from the transaction due to races or whatnot. As long as callers\nget a list of all dropped refs, including why they have been dropped,\nthey can also still communicate this information to the user.\n\n> For the files-backend code, I think system errors would naturally fall\n> out in the same code. Failing to write() or rename() is not much\n> different than failing to get the lock in the first place. So\n> \"partial-update\" and \"non-atomic\" behavior would end up the same anyway,\n> and we do not need to expose the third mode to the user.\n\nI think I disagree here. Failing to write() to me is quite different\nfrom failing to take a lock: the first one is an unexpected system-level\nerror and brings us into a situation where we ain't got no clue why it\nhappened. The second one is a logical error that is entirely expected\ngiven that lockfiles are explicitly designed for this failure mode, so\nwe know why they happen. With this in mind, I'd argue that we should\nonly continue with the transaction in the latter case, and abort on\nunexpected system-level errors.\n\n> For the reftable backend, syscalls are inherently covering all the refs\n> anyway (we either commit the whole reftable update or not). So likewise\n> there would be no different between partial-update and non-atomic modes\n> (but they would both be different from the files backend).\n> \n> I suspect the surgery needed for the ref-transaction code to allow\n> non-atomic updates would be pretty big, though. It involves checking\n> every error case to make sure it is safe to continue rather than\n> aborting (and munging data structures to mark particular refs as\n> \"failed, don't do anything further for this one\").\n\nI hope that it's not going to be that bad if we restrict it to the\n\"prepare\" phase, but that may just be wishful thinking.\n\n> So I dunno. All of my analysis assumes the breakdown of user\n> expectations I gave above is a reasonable one. There may be others. But\n> it seems like the behavior created by just this patch would be very hard\n> to explain, and subject to change based on implementation details.\n> \n> -Peff\n\nI think your analysis makes sense.\n\nWhile I think that the three modes aren't as bad for a command like\ngit-fetch(1), the \"single-tx-partial-update\" mode would really only be a\nspecial case of our current default, except that we use a single\ntransaction, only. Users shouldn't really need to care about this, and\nwe should do the right thing by default. The discussion mostly came to\nlive because our current implementation of reference transactions is\nlacking the ability to handle this partial-update mode well (and it\ndidn't really have to until now), but pushing this technical limitation\nonto the user is probably nothing we should do.\n\nIn the end I agree that we ought to extend the reftx mechanism to\nsupport this usecase. While it's more work up-front, I expect that other\ncommands could benefit in a similar way without having to add\n`--partial-atomic-reference-updates` switches to all of them.\n\nPatrick\n"},{"id":"443700","messageId":"xmqqzgp9o379.fsf@gitster.g","threadId":"57046","inReplyTo":"259de62b26302c10f429d52bec42a8a954bc5ba3.1638886972.git.ps@pks.im","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T21:53:14Z","receivedAt":"2021-12-09T21:53:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> instead of creating one slice per updated ref. While this inefficiency\n> can be easily mitigated by using the `--atomic` flag, this flag cannot\n> be used in contexts where we want partial-update semantics.\n\nInteresting and puzzling.  In today's code, we use a single\ntransaction when \"atomic\" is asked for, so that we can open a\ntransaction, prepare bunch of ref updates, and say \"commit\" to\ncommit all of them and let the ref_transaction layer to make the\nwhole thing all-or-none.  If we now use a single transaction for two\nrefs update that do not have to be atomic, it is surprising (from\nthe diffstat) that we can do so without changing anything in the\nref_transaction layer.  Doesn't the caller at least need to say\n\"this transaction is best-effort 'partial-update' (whatever it\nmeans)\" vs \"this transaction is all-or-none\"?  And doesn't the\nref_transaction layer now need to implement the 'partial-update'\nthing?\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/fetch.c | 78 ++++++++++++++++---------------------------------\n>  1 file changed, 25 insertions(+), 53 deletions(-)\n\n"},{"id":"443705","messageId":"xmqqtufho246.fsf@gitster.g","threadId":"57046","inReplyTo":"YbEgDBJPuGXQ+2t6@coredump.intra.peff.net","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-12-09T22:16:41Z","receivedAt":"2021-12-09T22:16:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I suspect the surgery needed for the ref-transaction code to allow\n> non-atomic updates would be pretty big, though. It involves checking\n> every error case to make sure it is safe to continue rather than\n> aborting (and munging data structures to mark particular refs as\n> \"failed, don't do anything further for this one\").\n>\n> So I dunno. All of my analysis assumes the breakdown of user\n> expectations I gave above is a reasonable one. There may be others. But\n> it seems like the behavior created by just this patch would be very hard\n> to explain, and subject to change based on implementation details.\n\nOh, I should have read your analysis before reacting to the original\nmessage myself ;-)\n\nYes, current callers of ref-transaction code may have some logic to\ndecide that it is not even worth telling a proposed change to the\nref API (e.g. non-fast-forward) but that does feel like an\nunnecessary implementation detail, and a true \"partial transaction\"\nneeds cooperation by the ref-transaction layer.\n\nAnd when it is done, we do not have to explain anything to the user\nbeyond what we already do.  The \"--atomic\" option will make it\nall-or-none, and without it, changes to each ref may or may not fail\nindividually with its own reason, without correlation to the outcome\nof the changes to any other refs.  And use of single or multiple\ntransactions just becomes an implementation detail of non-atomic\nupdates.\n"},{"id":"443756","messageId":"YbMYAA1EuFX4Z/5x@coredump.intra.peff.net","threadId":"57046","inReplyTo":"YbGsJnTQov6byXzF@ncase","subject":"Re: [RFC] fetch: update refs in a single transaction","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-12-10T09:04:00Z","receivedAt":"2021-12-10T09:04:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 09, 2021 at 08:11:34AM +0100, Patrick Steinhardt wrote:\n\n> > For the files-backend code, I think system errors would naturally fall\n> > out in the same code. Failing to write() or rename() is not much\n> > different than failing to get the lock in the first place. So\n> > \"partial-update\" and \"non-atomic\" behavior would end up the same anyway,\n> > and we do not need to expose the third mode to the user.\n> \n> I think I disagree here. Failing to write() to me is quite different\n> from failing to take a lock: the first one is an unexpected system-level\n> error and brings us into a situation where we ain't got no clue why it\n> happened. The second one is a logical error that is entirely expected\n> given that lockfiles are explicitly designed for this failure mode, so\n> we know why they happen. With this in mind, I'd argue that we should\n> only continue with the transaction in the latter case, and abort on\n> unexpected system-level errors.\n\nJust to be clear, it's not that I necessarily think these error types\nare logically related. I only meant that once you are detecting and\nrecovering from one type, it would be easy to implement it either way.\n\nI'd be OK with either type of behavior. If that was the only difference\nbetween partial-update and non-atomic, though, I'm not sure if that\nmerits exposing the complexity of a \"third mode\" to the user. But I\ndon't feel strongly about it either way.\n\n> > I suspect the surgery needed for the ref-transaction code to allow\n> > non-atomic updates would be pretty big, though. It involves checking\n> > every error case to make sure it is safe to continue rather than\n> > aborting (and munging data structures to mark particular refs as\n> > \"failed, don't do anything further for this one\").\n> \n> I hope that it's not going to be that bad if we restrict it to the\n> \"prepare\" phase, but that may just be wishful thinking.\n\nYeah, maybe. :) I didn't look closely, so it may not be too bad. I just\nremember the refs system being very finicky about things like failure,\nraces, etc. But I'm sure you'll figure it out once you start looking\nclosely. :)\n\nOne thing to watch out for is that in the files backend, _part_ of the\nupdate may be shared by multiple refs: updating the packed-refs file (if\nwe are deleting refs). So if you are deleting \"refs/heads/foo\" and\n\"refs/heads/bar\", but taking the lock on \"foo\" fails, you'd want to make\nsure only to delete \"bar\" from packed-refs, not \"foo\".\n\n-Peff\n"}]}