Re: [PATCH 08/10] reftable/reader: keep readers alive during iteration

[Patrick Steinhardt <ps@pks.im>]

On Fri, Aug 23, 2024 at 03:21:27AM -0700, karthik nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > diff --git a/reftable/stack_test.c b/reftable/stack_test.c
> > index bc3bf77274..91e716dc0a 100644
> > --- a/reftable/stack_test.c
> > +++ b/reftable/stack_test.c
> > @@ -1076,6 +1076,54 @@ static void test_reftable_stack_compaction_concurrent_clean(void)
> >  	clear_dir(dir);
> >  }
> >
> > +static void test_reftable_stack_read_across_reload(void)
> > +{
> > +	struct reftable_write_options opts = { 0 };
> > +	struct reftable_stack *st1 = NULL, *st2 = NULL;
> > +	struct reftable_ref_record rec = { 0 };
> > +	struct reftable_iterator it = { 0 };
> > +	char *dir = get_tmp_dir(__LINE__);
> > +	int err;
> > +
> > +	/* Create a first stack and set up an iterator for it. */
> > +	err = reftable_new_stack(&st1, dir, &opts);
> > +	EXPECT_ERR(err);
> > +	write_n_ref_tables(st1, 2);
> > +	EXPECT(st1->merged->readers_len == 2);
> > +	reftable_stack_init_ref_iterator(st1, &it);
> > +	err = reftable_iterator_seek_ref(&it, "");
> > +	EXPECT_ERR(err);
> > +
> > +	/* Set up a second stack for the same directory and compact it. */
> > +	err = reftable_new_stack(&st2, dir, &opts);
> > +	EXPECT_ERR(err);
> > +	EXPECT(st2->merged->readers_len == 2);
> > +	err = reftable_stack_compact_all(st2, NULL);
> 
> Shouldn't we verify that `EXPECT(st2->merged->readers_len == 1);` here?

Yes, we should.

> > +	EXPECT_ERR(err);
> > +
> > +	/*
> > +	 * Verify that we can continue to use the old iterator even after we
> > +	 * have reloaded its stack.
> > +	 */
> > +	err = reftable_stack_reload(st1);
> > +	EXPECT_ERR(err);
> > +	EXPECT(st2->merged->readers_len == 1);
> 
> Oh we do it here, I would've expected it above the `st1` reload. And
> probably expected `EXPECT(st1->merged->readers_len == 2);` here to
> confirm that we still have the readers.

And yes, this was a typo, it should've been `st1` indeed. Good eyes,
thanks!

Patrick