On Thu, Mar 14, 2024 at 6:55 PM John Naylor <johncnaylo...@gmail.com> wrote: > > On Thu, Mar 14, 2024 at 12:06 PM Masahiko Sawada <sawada.m...@gmail.com> > wrote: > > > > On Thu, Mar 14, 2024 at 1:29 PM John Naylor <johncnaylo...@gmail.com> wrote: > > > Okay, here's an another idea: Change test_lookup_tids() to be more > > > general and put the validation down into C as well. First we save the > > > blocks from do_set_block_offsets() into a table, then with all those > > > blocks lookup a sufficiently-large range of possible offsets and save > > > found values in another array. So the static items structure would > > > have 3 arrays: inserts, successful lookups, and iteration (currently > > > the iteration output is private to check_set_block_offsets(). Then > > > sort as needed and check they are all the same. > > > > That's a promising idea. We can use the same mechanism for randomized > > tests too. If you're going to work on this, I'll do other tests on my > > environment in the meantime. > > Some progress on this in v72 -- I tried first without using SQL to > save the blocks, just using the unique blocks from the verification > array. It seems to work fine.
Thanks! > > - Since there are now three arrays we should reduce max bytes to > something smaller. Agreed. > - Further on that, I'm not sure if the "is full" test is telling us > much. It seems we could make max bytes a static variable and set it to > the size of the empty store. I'm guessing it wouldn't take much to add > enough tids so that the contexts need to allocate some blocks, and > then it would appear full and we can test that. I've made it so all > arrays repalloc when needed, just in case. How about using work_mem as max_bytes instead of having it as a static variable? In test_tidstore.sql we set work_mem before creating the tidstore. It would make the tidstore more controllable by SQL queries. > - Why are we switching to TopMemoryContext? It's not explained -- the > comment only tells what the code is doing (which is obvious), but not > why. This is because the tidstore needs to live across the transaction boundary. We can use TopMemoryContext or CacheMemoryContext. > - I'm not sure it's useful to keep test_lookup_tids() around. Since we > now have a separate lookup test, the only thing it can tell us is that > lookups fail on an empty store. I arranged it so that > check_set_block_offsets() works on an empty store. Although that's > even more trivial, it's just reusing what we already need. Agreed. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com