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

[Junio C Hamano <gitster@pobox.com>]

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.



> 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;