Re: [PATCH 2/5] reftable/stack: add function to check if optimization is required

[Justin Tobler <jltobler@gmail.com>]

On 25/10/31 03:22PM, Karthik Nayak wrote:
> The reftable backend, performs auto-compaction as part of its regular
> flow, which is required to keep the number of tables part of a stack at
> bay. This allows it to stay optimized.
> 
> Compaction can also be triggered voluntarily by the user via the 'git
> pack-refs' or the 'git refs optimize' command. However, currently there
> is no way for the user to check if optimization is required without
> actually performing it.
> 
> Add and expose `reftable_stack_compaction_required()` which will allow
> users to check if the reftable backend can be optimized.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  reftable/reftable-stack.h       |  5 +++++
>  reftable/stack.c                | 25 +++++++++++++++++++++++++
>  t/unit-tests/u-reftable-stack.c | 12 ++++++++++--
>  3 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/reftable/reftable-stack.h b/reftable/reftable-stack.h
> index d70fcb705d..a875149439 100644
> --- a/reftable/reftable-stack.h
> +++ b/reftable/reftable-stack.h
> @@ -123,6 +123,11 @@ struct reftable_log_expiry_config {
>  int reftable_stack_compact_all(struct reftable_stack *st,
>  			       struct reftable_log_expiry_config *config);
>  
> +/* Check if compaction is required. */
> +int reftable_stack_compaction_required(struct reftable_stack *st,
> +				       bool use_heuristics,
> +				       bool *required);
> +
>  /* heuristically compact unbalanced table stack. */
>  int reftable_stack_auto_compact(struct reftable_stack *st);
>  
> diff --git a/reftable/stack.c b/reftable/stack.c
> index 49387f9344..18fa41cd5c 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -1647,6 +1647,31 @@ static int stack_segments_for_compaction(struct reftable_stack *st,
>  	return 0;
>  }
>  
> +int reftable_stack_compaction_required(struct reftable_stack *st,
> +				       bool use_heuristics,
> +				       bool *required)
> +{
> +	struct segment seg;
> +	int err = 0;
> +
> +	if (st->merged->tables_len < 2) {
> +		*required = false;
> +		return 0;
> +	}

Both `reftable_stack_auto_compact()` and `suggest_compaction_segement()`
already check if the stack has less than two tables. I wonder if we can
avoid having multiple of these checks by instead having a single one at
the start of `stack_segements_for_compaction()`?

> +	if (!use_heuristics) {
> +		*required = true;
> +		return 0;
> +	}

Is there a reason we would want to skip validating the geometric
sequence and just assume it compaction is required?

> +
> +	err = stack_segments_for_compaction(st, &seg);
> +	if (err)
> +		return err;
> +
> +	*required = segment_size(&seg) > 0;

As mentioned on the previous patch, I wonder if we could just return the
number of tables in the compaction segment as part of
`stack_segments_for_compaction()`. A negative value could indicate an
error. All other values would reflect the number of tables to be
compacted.

This way callers interested in whether compaction should be performed
could just do: stack_segments_for_compaction > 0. We could maybe avoid
having a separate function like we do here and just expose
`stack_segments_for_compaction()`.

-Justin