From: Justin Tobler <jltobler@gmail.com>
Date: Fri, 31 Oct 2025 16:22:56 GMT
Subject: Re: [PATCH 1/5] reftable/stack: return stack segments directly
Message-ID: <7gjrsjgi32akawqwcamzil2rblqelfvgmrxmgef5ssrslntmc6@43cra6zhledc>
In-Reply-To: <20251031-562-add-sub-command-to-check-if-maintenance-is-needed-v1-1-a03d53e28d0e@gmail.com>

On 25/10/31 03:22PM, Karthik Nayak wrote:
> The `stack_table_sizes_for_compaction()` function returns individual
> sizes of each reftable table. This function is only called by
> `reftable_stack_auto_compact()` to decide which tables need to be
> compacted, if any.

`stack_table_sizes_for_compaction()` provides the sizes of tables which
gets used by `suggest_compaction_segment()` to figure out the range of
tables that need to be compacted in order to restore the geometric
sequence. `reftable_stack_auto_compact()` coordinates invoking these two
functions and actually performs the compaction via
`stack_compact_range()`.

> Modify the function to directly return the segments, which avoids the
> extra step of receiving the sizes only to pass it to
> `suggest_compaction_segment()`.

Ok, so we want `suggest_compaction_segment()` to be invoked by
`stack_table_sizes_for_compaction()` instead of
`reftable_stack_auto_compact()`. So we are not really avoiding this
step, but just changing where it occurs.

> A future commit will also add functionality for checking whether
> auto-compaction is necessary without performing it. This change allows
> code re-usability in that context.

Makes sense.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  reftable/stack.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 65d89820bd..49387f9344 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1626,7 +1626,8 @@ struct segment suggest_compaction_segment(uint64_t *sizes, size_t n,
>  	return seg;
>  }
>  
> -static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
> +static int stack_segments_for_compaction(struct reftable_stack *st,
> +					 struct segment *seg)

`stack_segements_for_compaction()` now handles both getting the table
sizes and getting the segment range for compaction.

>  {
>  	int version = (st->opts.hash_id == REFTABLE_HASH_SHA1) ? 1 : 2;
>  	int overhead = header_size(version) - 1;
> @@ -1634,29 +1635,29 @@ static uint64_t *stack_table_sizes_for_compaction(struct reftable_stack *st)
>  
>  	REFTABLE_CALLOC_ARRAY(sizes, st->merged->tables_len);
>  	if (!sizes)
> -		return NULL;
> +		return REFTABLE_OUT_OF_MEMORY_ERROR;
>  
>  	for (size_t i = 0; i < st->merged->tables_len; i++)
>  		sizes[i] = st->tables[i]->size - overhead;
>  
> -	return sizes;
> +	*seg = suggest_compaction_segment(sizes, st->merged->tables_len,
> +					  st->opts.auto_compaction_factor);
> +	reftable_free(sizes);
> +
> +	return 0;
>  }
>  
>  int reftable_stack_auto_compact(struct reftable_stack *st)
>  {
>  	struct segment seg;
> -	uint64_t *sizes;
> +	int err;
>  
>  	if (st->merged->tables_len < 2)
>  		return 0;
>  
> -	sizes = stack_table_sizes_for_compaction(st);
> -	if (!sizes)
> -		return REFTABLE_OUT_OF_MEMORY_ERROR;
> -
> -	seg = suggest_compaction_segment(sizes, st->merged->tables_len,
> -					 st->opts.auto_compaction_factor);
> -	reftable_free(sizes);
> +	err = stack_segments_for_compaction(st, &seg);
> +	if (err)
> +		return err;

Looks good.

>  
>  	if (segment_size(&seg) > 0)
>  		return stack_compact_range(st, seg.start, seg.end - 1,

Do we expect the errors returned by `stack_segments_for_compaction()` to
always be negative? If so, I wonder if we should also have it return the
number of tables in the segment. That way it could also handle the
followup `segment_size()`.

-Justin