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

On Thu, Jan 12, 2023 at 5:21 PM John Naylor
<john(dot)naylor(at)enterprisedb(dot)com> wrote:
>
> On Thu, Jan 12, 2023 at 12:44 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Wed, Jan 11, 2023 at 12:13 PM John Naylor
> > <john(dot)naylor(at)enterprisedb(dot)com> wrote:
> > >
> > > On Tue, Jan 10, 2023 at 7:08 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> > I agree to keep this as a template.
>
> Okay, I'll squash the previous patch and work on cleaning up the internals. I'll keep the external APIs the same so that your work on vacuum integration can be easily rebased on top of that, and we can work independently.

Thanks!

>
> > From the vacuum integration
> > perspective, it would be better if we can use a common data type for
> > shared and local. It makes sense to have different data types if the
> > radix trees have different values types.
>
> I agree it would be better, all else being equal. I have some further thoughts below.
>
> > > > It looks no problem in terms of vacuum integration, although I've not
> > > > fully tested yet. TID store uses the radix tree as the main storage,
> > > > and with the template radix tree, the data types for shared and
> > > > non-shared will be different. TID store can have an union for the
> > > > radix tree and the structure would be like follows:
> > >
> > > > /* Storage for Tids */
> > > > union tree
> > > > {
> > > > local_radix_tree *local;
> > > > shared_radix_tree *shared;
> > > > };
> > >
> > > We could possibly go back to using a common data type for this, but with unused fields in each setting, as before. We would have to be more careful of things like the 32-bit crash from a few weeks ago.
> >
> > One idea to have a common data type without unused fields is to use
> > radix_tree a base class. We cast it to radix_tree_shared or
> > radix_tree_local depending on the flag is_shared in radix_tree. For
> > instance we have like (based on non-template version),
>
> > struct radix_tree
> > {
> > bool is_shared;
> > MemoryContext context;
> > };
>
> That could work in principle. My first impression is, just a memory context is not much of a base class. Also, casts can creep into a large number of places.
>
> Another thought came to mind: I'm guessing the TID store is unusual -- meaning most uses of radix tree will only need one kind of memory (local/shared). I could be wrong about that, and it _is_ a guess about the future. If true, then it makes more sense that only code that needs both memory kinds should be responsible for keeping them separate.

True.

>
> The template might be easier for future use cases if shared memory were all-or-nothing, meaning either
>
> - completely different functions and types depending on RT_SHMEM
> - use branches (like v8)
>
> The union sounds like a good thing to try, but do whatever seems right.

I've implemented the idea of using union. Let me share WIP code for
discussion, I've attached three patches that can be applied on top of
v17-0009 patch. v17-0010 implements missing shared memory support
functions such as RT_DETACH and RT_GET_HANDLE, and some fixes.
v17-0011 patch adds TidStore, and v17-0012 patch is the vacuum
integration.

Overall, TidStore implementation with the union idea doesn't look so
ugly to me. But I got many compiler warning about unused radix tree
functions like:

tidstore.c:99:19: warning: 'shared_rt_delete' defined but not used
[-Wunused-function]

I'm not sure there is a convenient way to suppress this warning but
one idea is to have some macros to specify what operations are
enabled/declared.

Regards,

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