Re: [PATCH 3/4] reftable/stack: register lockfiles during compaction

From: Justin Tobler <jltobler@gmail.com>
Date: Mar 5, 2024, 23:30 UTC
Message-ID: <mwhby7dxpiyrvknqe2uoli4ulygjy6hbxcxpqt3alw3dthwntr@4o24tp5jp6h7>
In-Reply-To: <45b5c3167fbfd64d8d1e14ed55bae94cb9cba28b.1709549619.git.ps@pks.im>

On 24/03/04 12:10PM, Patrick Steinhardt wrote:

Show 18 quoted lines

> We do not register any of the locks we acquire when compacting the
> reftable stack via our lockfiles interfaces. These locks will thus not
> be released when Git gets killed.
> 
> Refactor the code to register locks as lockfiles.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> ...
> +	/*
> +	 * Write the new "tables.list" contents with the compacted table we
> +	 * have just written. In case the compacted table became empty we
> +	 * simply skip writing it.
> +	 */
> +	for (i = 0; i < first; i++)
> +		strbuf_addf(&tables_list_buf, "%s\n", st->readers[i]->name);
> +	if (!is_empty_table)
> +		strbuf_addf(&tables_list_buf, "%s\n", new_table_name.buf);

Something not really related to this patch, but I noticed and had a question about.

If I'm understanding this correctly, when a newly compacted table is empty, it becomes possible for a range of indexes to no longer exist within the stack. If this occurs in the middle of the stack, future compaction will likely combine the tables on either side and restore the missing index range. If the empty table was at the end of the stack, would this effectly reset the max index to something lower for future tables written to the stack? If so, could this lead to issues with separate concurrent table writes?

Show 14 quoted lines

> ...  
> diff --git a/reftable/system.h b/reftable/system.h
> index 6b74a81514..5d8b6dede5 100644
> --- a/reftable/system.h
> +++ b/reftable/system.h
> @@ -12,7 +12,9 @@ license that can be found in the LICENSE file or at
>  /* This header glues the reftable library to the rest of Git */
>  
>  #include "git-compat-util.h"
> +#include "lockfile.h"
>  #include "strbuf.h"
> +#include "tempfile.h"
>  #include "hash-ll.h" /* hash ID, sizes.*/
>  #include "dir.h" /* remove_dir_recursively, for tests.*/

Naive question, why do we include the headers in `system.h`? I assume this is because they are common? Are there other benefits to this indirection?

-Justin