On Tue, Mar 5, 2024 at 11:12 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Tue, Mar 5, 2024 at 6:41 PM John Naylor <johncnaylo...@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.
