Re: [PoC] Improve dead tuple storage for lazy vacuum

On Thu, Feb 15, 2024 at 8:26 PM John Naylor <johncnaylorls@gmail.com> wrote:
>
> On Thu, Feb 15, 2024 at 10:21 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Sat, Feb 10, 2024 at 9:29 PM John Naylor <johncnaylorls@gmail.com> wrote:
>
> > I've also run the same scripts in my environment just in case and got
> > similar results:
>
> Thanks for testing, looks good as well.
>
> > > There are still some micro-benchmarks we could do on tidstore, and
> > > it'd be good to find out worse-case memory use (1 dead tuple each on
> > > spread-out pages), but this is decent demonstration.
> >
> > I've tested a simple case where vacuum removes 33k dead tuples spread
> > about every 10 pages.
> >
> > master:
> > 198,000 bytes (=33000 * 6)
> > system usage: CPU: user: 29.49 s, system: 0.88 s, elapsed: 30.40 s
> >
> > v-59:
> > 2,834,432 bytes (reported by TidStoreMemoryUsage())
> > system usage: CPU: user: 15.96 s, system: 0.89 s, elapsed: 16.88 s
>
> The memory usage for the sparse case may be a concern, although it's
> not bad -- a multiple of something small is probably not huge in
> practice. See below for an option we have for this.
>
> > > > > I'm pretty sure there's an
> > > > > accidental memset call that crept in there, but I'm running out of
> > > > > steam today.
> > >
> > > I have just a little bit of work to add for v59:
> > >
> > > v59-0009 - set_offset_bitmap_at() will call memset if it needs to zero
> > > any bitmapwords. That can only happen if e.g. there is an offset > 128
> > > and there are none between 64 and 128, so not a huge deal but I think
> > > it's a bit nicer in this patch.
> >
> > LGTM.
>
> Okay, I've squashed this.
>
> > I've drafted the commit message.
>
> Thanks, this is a good start.
>
> > I've run regression tests with valgrind and run the coverity scan, and
> > I don't see critical issues.
>
> Great!
>
> Now, I think we're in pretty good shape. There are a couple of things
> that might be objectionable, so I want to try to improve them in the
> little time we have:
>
> 1. Memory use for the sparse case. I shared an idea a few months ago
> of how runtime-embeddable values (true combined pointer-value slots)
> could work for tids. I don't think this is a must-have, but it's not a
> lot of code, and I have this working:
>
> v61-0006: Preparatory refactoring -- I think we should do this anyway,
> since the intent seems more clear to me.

Looks good refactoring to me.

> v61-0007: Runtime-embeddable tids -- Optional for v17, but should
> reduce memory regressions, so should be considered. Up to 3 tids can
> be stored in the last level child pointer. It's not polished, but I'll
> only proceed with that if we think we need this. "flags" iis called
> that because it could hold tidbitmap.c booleans (recheck, lossy) in
> the future, in addition to reserving space for the pointer tag. Note:
> I hacked the tests to only have 2 offsets per block to demo, but of
> course both paths should be tested.

Interesting. I've run the same benchmark tests we did[1][2] (the
median of 3 runs):

monotonically ordered int column index:

master: system usage: CPU: user: 14.91 s, system: 0.80 s, elapsed: 15.73 s
v-59: system usage: CPU: user: 9.67 s, system: 0.81 s, elapsed: 10.50 s
v-62: system usage: CPU: user: 1.94 s, system: 0.69 s, elapsed: 2.64 s

uuid column index:

master: system usage: CPU: user: 28.37 s, system: 1.38 s, elapsed: 29.81 s
v-59: system usage: CPU: user: 14.84 s, system: 1.31 s, elapsed: 16.18 s
v-62: system usage: CPU: user: 4.06 s, system: 0.98 s, elapsed: 5.06 s

int & uuid indexes in parallel:

master: system usage: CPU: user: 15.92 s, system: 1.39 s, elapsed: 34.33 s
v-59: system usage: CPU: user: 10.92 s, system: 1.20 s, elapsed: 17.58 s
v-62: system usage: CPU: user: 2.54 s, system: 0.94 s, elapsed: 6.00 s

sparse case:

master:
198,000 bytes (=33000 * 6)
system usage: CPU: user: 29.49 s, system: 0.88 s, elapsed: 30.40 s

v-59:
2,834,432 bytes (reported by TidStoreMemoryUsage())
system usage: CPU: user: 15.96 s, system: 0.89 s, elapsed: 16.88 s

v-62:
729,088 bytes (reported by TidStoreMemoryUsage())
system usage: CPU: user: 4.63 s, system: 0.86 s, elapsed: 5.50 s

I'm happy to see a huge improvement. While it's really fascinating to
me, I'm concerned about the time left until the feature freeze. We
need to polish both tidstore and vacuum integration patches in 5
weeks. Personally I'd like to have it as a separate patch for now, and
focus on completing the main three patches since we might face some
issues after pushing these patches. I think with 0007 patch it's a big
win but it's still a win even without 0007 patch.

>
> 2. Management of memory contexts. It's pretty verbose and messy. I
> think the abstraction could be better:
> A: tidstore currently passes CurrentMemoryContext to RT_CREATE, so we
> can't destroy or reset it. That means we have to do a lot of manual
> work.
> B: Passing "max_bytes" to the radix tree was my idea, I believe, but
> it seems the wrong responsibility. Not all uses will have a
> work_mem-type limit, I'm guessing. We only use it for limiting the max
> block size, and aset's default 8MB is already plenty small for
> vacuum's large limit anyway. tidbitmap.c's limit is work_mem, so
> smaller, and there it makes sense to limit the max blocksize this way.
> C: The context for values has complex #ifdefs based on the value
> length/varlen, but it's both too much and not enough. If we get a bump
> context, how would we shoehorn that in for values for vacuum but not
> for tidbitmap?
>
> Here's an idea: Have vacuum (or tidbitmap etc.) pass a context to
> TidStoreCreate(), and then to RT_CREATE. That context will contain the
> values (for local mem), and the node slabs will be children of the
> value context. That way, measuring memory usage and free-ing can just
> call with this parent context, and let recursion handle the rest.
> Perhaps the passed context can also hold the radix-tree struct, but
> I'm not sure since I haven't tried it. What do you think?

If I understand your idea correctly, RT_CREATE() creates the context
for values as a child of the passed context and the node slabs as
children of the value context. That way, measuring memory usage can
just call with the value context. It sounds like a good idea. But it
was not clear to me how to address point B and C.

Another variant of this idea would be that RT_CREATE() creates the
parent context of the value context to store radix-tree struct. That
is, the hierarchy would be like:

A MemoryContext (passed by vacuum through tidstore)
    - radix tree memory context (store radx-tree struct, control
struct, and iterator)
        - value context (aset, slab, or bump)
            - node slab contexts

Freeing can just call with the radix tree memory context. And perhaps
it works even if tidstore passes CurrentMemoryContex to RT_CREATE()?

>
> With this resolved, I think the radix tree is pretty close to
> committable. The tid store will likely need some polish yet, but no
> major issues I know of.

Agreed.

>
> (And, finally, a small thing I that I wanted to share just so I don't
> forget, but maybe not worth the attention: In Andres's prototype,
> there is a comment wondering if an update can skip checking if it
> first need to create a root node. This is pretty easy, and done in
> v61-0008.)

LGTM, thanks!

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com