[v3,0/9] reftable: improvements and fixes for compaction

Message

Patrick Steinhardt Aug. 8, 2024, 2:06 p.m. UTC

Hi,

this is the second version of my patch series that aims to improve the
way reftable stack perform compaction.

Changes compared to v2:

  - Drop the unused `reftable_write_options` structure in
    `write_n_ref_tables()`.

  - Fix a commit message typo.

  - Reorder some variable assignments to feel more natural.

Thanks!

Patrick

Patrick Steinhardt (9):
  reftable/stack: refactor function to gather table sizes
  reftable/stack: extract function to setup stack with N tables
  reftable/stack: test compaction with already-locked tables
  reftable/stack: update stats on failed full compaction
  reftable/stack: simplify tracking of table locks
  reftable/stack: do not die when fsyncing lock file files
  reftable/stack: use lock_file when adding table to "tables.list"
  reftable/stack: fix corruption on concurrent compaction
  reftable/stack: handle locked tables during auto-compaction

 reftable/stack.c           | 231 +++++++++++++++++++++++++++++--------
 reftable/stack_test.c      | 142 ++++++++++++++++++-----
 t/t0610-reftable-basics.sh |  21 ++--
 3 files changed, 310 insertions(+), 84 deletions(-)

Range-diff against v2:
 1:  6d2b54ba8e =  1:  6d2b54ba8e reftable/stack: refactor function to gather table sizes
 2:  ff17306cc0 !  2:  798a661824 reftable/stack: extract function to setup stack with N tables
    @@ Commit message
         tests. This is fine though as we only care about the shape of the stack
         here, not the shape of each table.
     
    +    Furthermore, with this change we now start to disable auto compaction
    +    when writing the tables, as otherwise we might not end up with the
    +    expected amount of new tables added. This also slightly changes the
    +    behaviour of these tests, but the properties we care for remain intact.
    +
         Signed-off-by: Patrick Steinhardt <[e-mail]>
     
      ## reftable/stack_test.c ##
    @@ reftable/stack_test.c: static int write_test_ref(struct reftable_writer *wr, voi
      }
      
     +static void write_n_ref_tables(struct reftable_stack *st,
    -+			       struct reftable_write_options *opts,
     +			       size_t n)
     +{
     +	struct strbuf buf = STRBUF_INIT;
    ++	int disable_auto_compact;
     +	int err;
     +
    ++	disable_auto_compact = st->opts.disable_auto_compact;
    ++	st->opts.disable_auto_compact = 1;
    ++
     +	for (size_t i = 0; i < n; i++) {
     +		struct reftable_ref_record ref = {
     +			.update_index = reftable_stack_next_update_index(st),
    @@ reftable/stack_test.c: static int write_test_ref(struct reftable_writer *wr, voi
     +		EXPECT_ERR(err);
     +	}
     +
    ++	st->opts.disable_auto_compact = disable_auto_compact;
     +	strbuf_release(&buf);
     +}
     +
    @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent(voi
     -		err = reftable_stack_add(st1, &write_test_ref, &ref);
     -		EXPECT_ERR(err);
     -	}
    -+	write_n_ref_tables(st1, &opts, 3);
    ++	write_n_ref_tables(st1, 3);
      
      	err = reftable_new_stack(&st2, dir, &opts);
      	EXPECT_ERR(err);
    @@ reftable/stack_test.c: static void test_reftable_stack_compaction_concurrent_cle
     -		err = reftable_stack_add(st1, &write_test_ref, &ref);
     -		EXPECT_ERR(err);
     -	}
    -+	write_n_ref_tables(st1, &opts, 3);
    ++	write_n_ref_tables(st1, 3);
      
      	err = reftable_new_stack(&st2, dir, &opts);
      	EXPECT_ERR(err);
 3:  63e64c8d82 !  3:  949f748823 reftable/stack: test compaction with already-locked tables
    @@ reftable/stack_test.c: static void test_reftable_stack_auto_compaction(void)
     +	err = reftable_new_stack(&st, dir, &opts);
     +	EXPECT_ERR(err);
     +
    -+	write_n_ref_tables(st, &opts, 5);
    ++	write_n_ref_tables(st, 5);
     +	EXPECT(st->merged->stack_len == 5);
     +
     +	/*
    @@ reftable/stack_test.c: static void test_reftable_stack_add_performs_auto_compact
     +	err = reftable_new_stack(&st, dir, &opts);
     +	EXPECT_ERR(err);
     +
    -+	write_n_ref_tables(st, &opts, 3);
    ++	write_n_ref_tables(st, 3);
     +	EXPECT(st->merged->stack_len == 3);
     +
     +	/* Lock one of the tables that we're about to compact. */
 4:  1989dafcb4 =  4:  478f945a45 reftable/stack: update stats on failed full compaction
 5:  73e5d104eb =  5:  812a45f3d2 reftable/stack: simplify tracking of table locks
 6:  e411e14904 =  6:  d7d34e7253 reftable/stack: do not die when fsyncing lock file files
 7:  b868a518d6 =  7:  37a757649a reftable/stack: use lock_file when adding table to "tables.list"
 8:  ff17414d26 !  8:  b27cb325fc reftable/stack: fix corruption on concurrent compaction
    @@ reftable/stack.c: static int stack_compact_range(struct reftable_stack *st,
     +		 * We have found the new range that we want to replace, so
     +		 * let's update the range of tables that we want to replace.
     +		 */
    -+		last_to_replace = last + (new_offset - first);
     +		first_to_replace = new_offset;
    ++		last_to_replace = last + (new_offset - first);
     +	} else {
     +		/*
     +		 * `fd_read_lines()` uses a `NULL` sentinel to indicate that
    @@ reftable/stack.c: static int stack_compact_range(struct reftable_stack *st,
     +		REFTABLE_CALLOC_ARRAY(names, st->merged->stack_len + 1);
     +		for (size_t i = 0; i < st->merged->stack_len; i++)
     +			names[i] = xstrdup(st->readers[i]->name);
    -+		last_to_replace = last;
     +		first_to_replace = first;
    ++		last_to_replace = last;
     +	}
     +
      	/*
 9:  1ef560feb1 !  9:  dc2fea145d reftable/stack: handle locked tables during auto-compaction
    @@ Commit message
         this situation by instead compacting tables which aren't locked. To do
         so, instead of locking from the beginning of the slice-to-be-compacted,
         we start locking tables from the end of the slice. Once we hit the first
    -    table that is locked already, we abort. If we succeded to lock two or
    +    table that is locked already, we abort. If we succeeded to lock two or
         more tables, then we simply reduce the slice of tables that we're about
         to compact to those which we managed to lock.

Comments

Karthik Nayak Aug. 8, 2024, 4:52 p.m. UTC | #1

Patrick Steinhardt <[e-mail]> writes:

> Hi,
>
> this is the second version of my patch series that aims to improve the
> way reftable stack perform compaction.
>
> Changes compared to v2:
>
>   - Drop the unused `reftable_write_options` structure in
>     `write_n_ref_tables()`.
>
>   - Fix a commit message typo.
>
>   - Reorder some variable assignments to feel more natural.
>
> Thanks!
>
> Patrick
>

This version includes all the suggestions I made on the previous
version. Looks good to me now!

Thanks

[snip]