Skip to content

reftable: fix quadratic behavior when re-creating deleted refs #2166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
spkrka wants to merge 2 commits into
base: master
Choose a base branch
from
+110 −27
Open

reftable: fix quadratic behavior when re-creating deleted refs #2166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d8ffdcb
t: add tests for ref tombstone scenarios
spkrka Jul 6, 2026
1459371
reftable: fix quadratic behavior when re-creating deleted refs
spkrka Jul 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 43 additions & 11 deletions refs/reftable-backend.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ static int reftable_backend_read_ref(struct reftable_backend *be,
if (ret)

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Jul 06, 2026 at 01:35:56PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
> 
> When many refs are deleted and then re-created, update-ref exhibits
> quadratic behavior.  With 8000 refs deleted and re-created, the
> runtime is ~15s, quadrupling for each doubling of input size.
> 
> The root cause is the merged iterator's suppress_deletions flag.
> When set, merged_iter_next_void() silently consumes tombstone records
> in a tight internal loop before returning to the caller.  This
> prevents higher-level code from checking iteration bounds (such as
> prefix or refname comparisons) until after all tombstones have been
> scanned.
> 
> This affects two code paths during ref creation:
> 
>  - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to
>    check for D/F conflicts and must scan through all subsequent
>    tombstones before the caller can see that they are past the prefix
>    of interest.
> 
>  - reftable_backend_read_ref() seeks to a specific refname and must
>    scan through all subsequent tombstones before returning "not
>    found", because the merged iterator skips the matching tombstone
>    and searches for the next live record.

It probably not only impacts reference creation, but also every reader
that wants to search for a specific reference that doesn't exist.

> Fix this by removing suppress_deletions from the merged iterator and
> instead handling deletion records at each call site in the reftable
> backend, where prefix and refname bounds are available.  Tombstones
> are now returned to callers, which skip them after their existing
> bounds checks.  This allows iteration to terminate as soon as a
> tombstone past the relevant bound is encountered.

This option is still used by downstream users of the reftable library,
like libgit2. So we shouldn't just delete it outright.

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 4ae22922de..8c4f119ff1 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  			break;
>  		}
>  
> +		if (iter->ref.value_type == REFTABLE_REF_DELETION)
> +			continue;
> +
>  		if (iter->exclude_patterns && should_exclude_current_ref(iter))
>  			continue;
>  

Okay. I was first wondering whether we should move this call earlier.
But we actually don't want to, as this is the code that precedes the
above:

	if (iter->prefix_len &&
	    strncmp(iter->prefix, iter->ref.refname, iter->prefix_len)) {
		iter->err = 1;
		break;
	}

So this allows us to not only skip the current iteration, but completely
abort iteration by observing tombstones that sort after our prefix.

In any case, as far as I can see all sites where we iterate through
either ref or log records have been adapted to handle deletions.

Thanks!

Patrick

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote:
>
> > This affects two code paths during ref creation:
> >
> >  - refs_verify_refnames_available() seeks to "refs/tags/foo-1/" to
> >    check for D/F conflicts and must scan through all subsequent
> >    tombstones before the caller can see that they are past the prefix
> >    of interest.
> >
> >  - reftable_backend_read_ref() seeks to a specific refname and must
> >    scan through all subsequent tombstones before returning "not
> >    found", because the merged iterator skips the matching tombstone
> >    and searches for the next live record.
>
> It probably not only impacts reference creation, but also every reader
> that wants to search for a specific reference that doesn't exist.

Hm good point, I will try to rephrase this better.

> > Fix this by removing suppress_deletions from the merged iterator and
> > instead handling deletion records at each call site in the reftable
> > backend, where prefix and refname bounds are available.  Tombstones
> > are now returned to callers, which skip them after their existing
> > bounds checks.  This allows iteration to terminate as soon as a
> > tombstone past the relevant bound is encountered.
>
> This option is still used by downstream users of the reftable library,
> like libgit2. So we shouldn't just delete it outright.

Good catch! I can keep suppress_deletions as-is and just
stop setting it from stack.c. That way libgit2 is unchanged, while
we still optimize it at the other call sites. The reftable library
diff then shrinks to a single removed line.

> > diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> > index 4ae22922de..8c4f119ff1 100644
> > --- a/refs/reftable-backend.c
> > +++ b/refs/reftable-backend.c
> > @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
> >                       break;
> >               }
> >
> > +             if (iter->ref.value_type == REFTABLE_REF_DELETION)
> > +                     continue;
> > +
> >               if (iter->exclude_patterns && should_exclude_current_ref(iter))
> >                       continue;
> >
>
> Okay. I was first wondering whether we should move this call earlier.
> But we actually don't want to, as this is the code that precedes the
> above:
>
>         if (iter->prefix_len &&
>             strncmp(iter->prefix, iter->ref.refname, iter->prefix_len)) {
>                 iter->err = 1;
>                 break;
>         }
>
> So this allows us to not only skip the current iteration, but completely
> abort iteration by observing tombstones that sort after our prefix.

Indeed, this is the primary win.

> In any case, as far as I can see all sites where we iterate through
> either ref or log records have been adapted to handle deletions.

Thanks! Appreciate the review (and spotting the libgit breakage!)
Kristofer

goto done;

if (strcmp(ref.refname, refname)) {
if (strcmp(ref.refname, refname) ||
reftable_ref_record_is_deletion(&ref)) {
ret = 1;
goto done;
}
Expand All @@ -112,7 +113,6 @@ static int reftable_backend_read_ref(struct reftable_backend *be,
oidread(oid, reftable_ref_record_val1(&ref),
&hash_algos[hash_id]);
} else {
/* We got a tombstone, which should not happen. */
BUG("unhandled reference value type %d", ref.value_type);
}

Expand Down Expand Up @@ -633,6 +633,9 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
break;
}

if (iter->ref.value_type == REFTABLE_REF_DELETION)
continue;

if (iter->exclude_patterns && should_exclude_current_ref(iter))
continue;

Expand Down Expand Up @@ -1492,6 +1495,8 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data
ret = 0;
break;
}
if (reftable_log_record_is_deletion(&log))
continue;

ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
tombstone = &logs[logs_nr++];
Expand Down Expand Up @@ -1889,6 +1894,8 @@ static int write_copy_table(struct reftable_writer *writer, void *cb_data)
ret = 0;
break;
}
if (reftable_log_record_is_deletion(&old_log))
continue;

free(old_log.refname);

Expand Down Expand Up @@ -2019,6 +2026,9 @@ static int reftable_reflog_iterator_advance(struct ref_iterator *ref_iterator)
if (iter->err)
break;

if (reftable_log_record_is_deletion(&iter->log))
continue;

/*
* We want the refnames that we have reflogs for, so we skip if
* we've already produced this name. This could be faster by
Expand Down Expand Up @@ -2178,6 +2188,8 @@ static int reftable_be_for_each_reflog_ent_reverse(struct ref_store *ref_store,
ret = 0;
break;
}
if (reftable_log_record_is_deletion(&log))
continue;

ret = yield_log_record(refs, &log, fn, cb_data);
if (ret)
Expand Down Expand Up @@ -2230,6 +2242,10 @@ static int reftable_be_for_each_reflog_ent(struct ref_store *ref_store,
ret = 0;
break;
}
if (reftable_log_record_is_deletion(&log)) {
reftable_log_record_release(&log);
continue;
}

ALLOC_GROW(logs, logs_nr + 1, logs_alloc);
logs[logs_nr++] = log;
Expand Down Expand Up @@ -2276,18 +2292,26 @@ static int reftable_be_reflog_exists(struct ref_store *ref_store,
goto done;

/*
* Check whether we get at least one log record for the given ref name.
* If so, the reflog exists, otherwise it doesn't.
* Check whether we get at least one non-deleted log record for the
* given ref name. If so, the reflog exists, otherwise it doesn't.
*/
ret = reftable_iterator_next_log(&it, &log);
if (ret < 0)
goto done;
if (ret > 0) {
ret = 0;
goto done;
while (1) {
ret = reftable_iterator_next_log(&it, &log);
if (ret < 0)
goto done;
if (ret > 0) {
ret = 0;
goto done;
}
if (strcmp(log.refname, refname)) {
ret = 0;
goto done;
}
if (!reftable_log_record_is_deletion(&log))
break;
}

ret = strcmp(log.refname, refname) == 0;
ret = 1;

done:
reftable_iterator_destroy(&it);
Expand Down Expand Up @@ -2399,6 +2423,8 @@ static int write_reflog_delete_table(struct reftable_writer *writer, void *cb_da
ret = 0;
break;
}
if (reftable_log_record_is_deletion(&log))
continue;

tombstone.refname = (char *)arg->refname;
tombstone.value_type = REFTABLE_LOG_DELETION;
Expand Down Expand Up @@ -2580,6 +2606,10 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,
reftable_log_record_release(&log);
break;
}
if (reftable_log_record_is_deletion(&log)) {
reftable_log_record_release(&log);
continue;
}

oidread(&old_oid, log.value.update.old_hash,
ref_store->repo->hash_algo);
Expand Down Expand Up @@ -2746,6 +2776,8 @@ static int reftable_be_fsck(struct ref_store *ref_store, struct fsck_options *o,
report.path = refname.buf;

switch (ref.value_type) {
case REFTABLE_REF_DELETION:
continue;
case REFTABLE_REF_VAL1:
case REFTABLE_REF_VAL2: {
struct object_id oid;
Expand Down
12 changes: 1 addition & 11 deletions reftable/merged.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ struct merged_iter {
struct merged_subiter *subiters;
struct merged_iter_pqueue pq;
size_t subiters_len;
int suppress_deletions;
ssize_t advance_index;
};

Expand Down Expand Up @@ -166,15 +165,7 @@ static int merged_iter_seek_void(void *it, struct reftable_record *want)

static int merged_iter_next_void(void *p, struct reftable_record *rec)
{
struct merged_iter *mi = p;
while (1) {
int err = merged_iter_next_entry(mi, rec);
if (err)
return err;
if (mi->suppress_deletions && reftable_record_is_deletion(rec))
continue;
return 0;
}
return merged_iter_next_entry(p, rec);
}

static struct reftable_iterator_vtable merged_iter_vtable = {
Expand Down Expand Up @@ -278,7 +269,6 @@ int merged_table_init_iter(struct reftable_merged_table *mt,
goto out;
}
mi->advance_index = -1;
mi->suppress_deletions = mt->suppress_deletions;
mi->subiters = subiters;
mi->subiters_len = mt->tables_len;

Expand Down
4 changes: 0 additions & 4 deletions reftable/merged.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,6 @@ struct reftable_merged_table {
size_t tables_len;
enum reftable_hash hash_id;

/* If unset, produce deletions. This is useful for compaction. For the
* full stack, deletions should be produced. */
int suppress_deletions;

uint64_t min;
uint64_t max;
};
Expand Down
1 change: 0 additions & 1 deletion reftable/stack.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st,
/* Update the stack to point to the new tables. */
if (st->merged)
reftable_merged_table_free(st->merged);
new_merged->suppress_deletions = 1;
st->merged = new_merged;

if (st->tables)
Expand Down
44 changes: 44 additions & 0 deletions t/perf/p1401-ref-store-tombstones.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
#!/bin/sh

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh
> new file mode 100755
> index 0000000000..e40a6dcbf4
> --- /dev/null
> +++ b/t/perf/p1401-ref-store-tombstones.sh
> @@ -0,0 +1,44 @@
> +#!/bin/sh
> +
> +test_description="Tests performance of ref operations with many tombstones"
> +
> +. ./perf-lib.sh
> +
> +test_expect_success "setup" '
> +	git init --ref-format=reftable repo &&
> +	blob=$(echo foo | git -C repo hash-object -w --stdin) &&
> +	for i in $(test_seq 8000)
> +	do
> +		printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
> +		return 1
> +	done >repo/input &&
> +	git -C repo update-ref --stdin <repo/input &&
> +	git -C repo for-each-ref --format="delete %(refname)" |
> +	git -C repo update-ref --stdin
> +'
> +
> +test_perf "recreate refs after mass delete" '
> +	git -C repo update-ref --stdin <repo/input &&
> +	git -C repo for-each-ref --format="delete %(refname)" |
> +	git -C repo update-ref --stdin
> +'

You're not only benchmarking the reference recreation, but also their
deletion. If I'm not misreading things, then you can queue cleanups via
`test_when_finished`, and these calls will not be measured.

> +test_expect_success "setup asymmetric" '
> +	for i in $(test_seq 8000)
> +	do
> +		printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
> +		return 1
> +	done >repo/input-old &&
> +	sed "s/old-/new-/" <repo/input-old >repo/input-new &&
> +	git -C repo update-ref --stdin <repo/input-old &&
> +	git -C repo for-each-ref --format="delete %(refname)" |
> +	git -C repo update-ref --stdin
> +'

Would it make sense to use separate repositories? Otherwise, state from
the preceding benchmark(s) will impact subsequent ones.

> diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> index e19e036898..4b7cfe38e4 100755
> --- a/t/t0610-reftable-basics.sh
> +++ b/t/t0610-reftable-basics.sh
> @@ -1163,4 +1163,26 @@ test_expect_success 'writes do not persist peeled value for invalid tags' '
>  	)
>  '
>  
> +test_expect_success 'delete and re-create refs with tombstones' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	test_commit -C repo A &&
> +	A=$(git -C repo rev-parse HEAD) &&
> +	cat >input <<-EOF &&
> +	create refs/tags/a $A
> +	create refs/tags/b $A
> +	create refs/tags/c $A
> +	EOF
> +	git -C repo update-ref --stdin <input &&
> +
> +	# delete all tags, leaving tombstones
> +	git -C repo for-each-ref --format="delete %(refname)" refs/tags/ |
> +	git -C repo update-ref --stdin &&
> +
> +	# re-create the same refs and verify they are visible
> +	git -C repo update-ref --stdin <input &&
> +	git -C repo tag -l >actual &&
> +	test_line_count = 3 actual
> +'

I wonder whether this test really adds any value. We probably have lots
of tests already that test creation/deletion of references.

Patrick

@gitgitgadget gitgitgadget Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):

On Tue, 7 Jul 2026 at 17:24, Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Jul 06, 2026 at 01:35:55PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > diff --git a/t/perf/p1401-ref-store-tombstones.sh b/t/perf/p1401-ref-store-tombstones.sh
> > new file mode 100755
> > index 0000000000..e40a6dcbf4
> > --- /dev/null
> > +++ b/t/perf/p1401-ref-store-tombstones.sh
> > @@ -0,0 +1,44 @@
> > +#!/bin/sh
> > +
> > +test_description="Tests performance of ref operations with many tombstones"
> > +
> > +. ./perf-lib.sh
> > +
> > +test_expect_success "setup" '
> > +     git init --ref-format=reftable repo &&
> > +     blob=$(echo foo | git -C repo hash-object -w --stdin) &&
> > +     for i in $(test_seq 8000)
> > +     do
> > +             printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
> > +             return 1
> > +     done >repo/input &&
> > +     git -C repo update-ref --stdin <repo/input &&
> > +     git -C repo for-each-ref --format="delete %(refname)" |
> > +     git -C repo update-ref --stdin
> > +'
> > +
> > +test_perf "recreate refs after mass delete" '
> > +     git -C repo update-ref --stdin <repo/input &&
> > +     git -C repo for-each-ref --format="delete %(refname)" |
> > +     git -C repo update-ref --stdin
> > +'
>
> You're not only benchmarking the reference recreation, but also their
> deletion. If I'm not misreading things, then you can queue cleanups via
> `test_when_finished`, and these calls will not be measured.

I don't think measuring the full create+delete cycle is wrong per se,
but you are right that if we can benchmark something more isolated
is even more useful. I will try to split this up better.

> > +test_expect_success "setup asymmetric" '
> > +     for i in $(test_seq 8000)
> > +     do
> > +             printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
> > +             return 1
> > +     done >repo/input-old &&
> > +     sed "s/old-/new-/" <repo/input-old >repo/input-new &&
> > +     git -C repo update-ref --stdin <repo/input-old &&
> > +     git -C repo for-each-ref --format="delete %(refname)" |
> > +     git -C repo update-ref --stdin
> > +'
>
> Would it make sense to use separate repositories? Otherwise, state from
> the preceding benchmark(s) will impact subsequent ones.

Agreed, I can use a fresh repo for each scenario.

>
> > diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
> > +test_expect_success 'delete and re-create refs with tombstones' '
>
> I wonder whether this test really adds any value. We probably have lots
> of tests already that test creation/deletion of references.

I could not find an existing test that covers the delete-then-recreate
flow (where tombstones are present when the new refs are created).
The existing tests cover creation and deletion separately but not the
interaction with tombstones.
(But perhaps such a test exists and I just can't find it.)

Thanks,
Kristofer


test_description="Tests performance of ref operations with many tombstones"

. ./perf-lib.sh

test_expect_success "setup" '
git init --ref-format=reftable repo &&
blob=$(echo foo | git -C repo hash-object -w --stdin) &&
for i in $(test_seq 8000)
do
printf "create refs/tags/tag-%d %s\n" "$i" "$blob" ||
return 1
done >repo/input &&
git -C repo update-ref --stdin <repo/input &&
git -C repo for-each-ref --format="delete %(refname)" |
git -C repo update-ref --stdin
'

test_perf "recreate refs after mass delete" '
git -C repo update-ref --stdin <repo/input &&
git -C repo for-each-ref --format="delete %(refname)" |
git -C repo update-ref --stdin
'

test_expect_success "setup asymmetric" '
for i in $(test_seq 8000)
do
printf "create refs/tags/old-%d %s\n" "$i" "$blob" ||
return 1
done >repo/input-old &&
sed "s/old-/new-/" <repo/input-old >repo/input-new &&
git -C repo update-ref --stdin <repo/input-old &&
git -C repo for-each-ref --format="delete %(refname)" |
git -C repo update-ref --stdin
'

test_perf "create new refs after deleting differently-named refs" '
git -C repo update-ref --stdin <repo/input-new &&
git -C repo for-each-ref --format="delete %(refname)" |
git -C repo update-ref --stdin
'

test_done
22 changes: 22 additions & 0 deletions t/t0610-reftable-basics.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,4 +1163,26 @@ test_expect_success 'writes do not persist peeled value for invalid tags' '
)
'

test_expect_success 'delete and re-create refs with tombstones' '
test_when_finished "rm -rf repo" &&
git init repo &&
test_commit -C repo A &&
A=$(git -C repo rev-parse HEAD) &&
cat >input <<-EOF &&
create refs/tags/a $A
create refs/tags/b $A
create refs/tags/c $A
EOF
git -C repo update-ref --stdin <input &&

# delete all tags, leaving tombstones
git -C repo for-each-ref --format="delete %(refname)" refs/tags/ |
git -C repo update-ref --stdin &&

# re-create the same refs and verify they are visible
git -C repo update-ref --stdin <input &&
git -C repo tag -l >actual &&
test_line_count = 3 actual
'

test_done
Loading