On Thu, Jan 12, 2023 at 12:44 PM Masahiko Sawada <sawada.m...@gmail.com>
wrote:
>
> On Wed, Jan 11, 2023 at 12:13 PM John Naylor
> <john.nay...@enterprisedb.com> wrote:
> >
> > On Tue, Jan 10, 2023 at 7:08 PM Masahiko Sawada <sawada.m...@gmail.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.
> 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.
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.
--
John Naylor
EDB: http://www.enterprisedb.com
