On Tue, Mar 5, 2024 at 11:12 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Mar 5, 2024 at 6:41 PM John Naylor <johncnaylorls@gmail.com> wrote:
> > Done in v66-0009. I'd be curious to hear any feedback. I like the
> > aspect that the random numbers come from a different seed every time
> > the test runs.
>
> The new tests look good. Here are some comments:
>
> ---
> + expected = keys[i];
> + iterval = rt_iterate_next(iter, &iterkey);
>
> - ndeleted++;
> + EXPECT_TRUE(iterval != NULL);
> + EXPECT_EQ_U64(iterkey, expected);
> + EXPECT_EQ_U64(*iterval, expected);
>
> Can we verify that the iteration returns keys in ascending order?
We get the "expected" value from the keys we saved in the now-sorted
array, so we do already. Unless I misunderstand you.
> ---
> + /* reset random number generator for deletion */
> + pg_prng_seed(&state, seed);
>
> Why is resetting the seed required here?
Good catch - My intention was to delete in the same random order we
inserted with. We still have the keys in the array, but they're sorted
by now. I forgot to go the extra step and use the prng when generating
the keys for deletion -- will fix.
> ---
> The radix tree (and dsa in TSET_SHARED_RT case) should be freed at the end.
Will fix.
> ---
> radixtree_ctx = AllocSetContextCreate(CurrentMemoryContext,
> "test_radix_tree",
> ALLOCSET_DEFAULT_SIZES);
>
> We use a mix of ALLOCSET_DEFAULT_SIZES and ALLOCSET_SMALL_SIZES. I
> think it's better to use either one for consistency.
Will change to "small", since 32-bit platforms will use slab for leaves.
I'll look at the memory usage and estimate what 32-bit platforms will
use, and maybe adjust the number of keys. A few megabytes is fine, but
not many megabytes.
> > I'd like to push 0001 and 0002 shortly, and then do another sweep over
> > 0003, with remaining feedback, and get that in so we get some
> > buildfarm testing before the remaining polishing work on
> > tidstore/vacuum.
>
> Sounds a reasonable plan. 0001 and 0002 look good to me. I'm going to
> polish tidstore and vacuum patches and update commit messages.
Sounds good.
