Re: [PATCH 1/4] reftable/stack: don't perform auto-compaction with less than two tables

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 21, 2024, 17:08 UTC
Message-ID: <xmqqmsgpc6bf.fsf@gitster.g>
In-Reply-To: <20241221-b4-pks-reftable-oom-fix-without-readers-v1-1-12db83a3267c@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> In order to compact tables we need at least two tables. Bail out early
> from `reftable_stack_auto_compact()` in case we have less than two
> tables.

While that is very true, bailing out on "< 2" would change the behaviour. Where there were only one table, earlier we still went ahead and exercised compaction code path, but now we no longer do. The end result of having just a single table might logically be the same with this change, but if we were relying on some side effects of exercising the compaction code path, do we know that the rest of the code is OK?

That's the kind of questions I would ask, if this were somebody who hasn't been deeply involved in the reftable code and came this deep in the pre-release period. But since we all know you have been the main driver for this effort, we'd take your word for it ;-)

Thanks, will queue.

Show 24 quoted lines

> This is mostly defense in depth: `stack_table_sizes_for_compaction()`
> may try to allocate a zero-byte object when there aren't any readers,
> and that may lead to a `NULL` pointer on some platforms like NonStop
> which causes us to bail out with an out-of-memory error.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  reftable/stack.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 59fd695a12c2033ed589a21ef1c9155eeecc4641..6ca21965d8e1135d986043113d465abd14cd532c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1627,6 +1627,9 @@ int reftable_stack_auto_compact(struct reftable_stack *st)
>  	struct segment seg;
>  	uint64_t *sizes;
>  
> +	if (st->merged->readers_len < 2)
> +		return 0;
> +
>  	sizes = stack_table_sizes_for_compaction(st);
>  	if (!sizes)
>  		return REFTABLE_OUT_OF_MEMORY_ERROR;