On Thu, Nov 26, 2020 at 9:55 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> +inline Size
> +GetTupleSize(TupleTableSlot *slot, Size maxsize)
> +{
> + Size sz = 0;
> + HeapTuple tuple = NULL;
> +
> + if (TTS_IS_HEAPTUPLE(slot))
> + tuple = ((HeapTupleTableSlot *) slot)->tuple;
> + else if(TTS_IS_BUFFERTUPLE(slot))
> + tuple = ((BufferHeapTupleTableSlot *) slot)->base.tuple;
> + else if(TTS_IS_MINIMALTUPLE(slot))
> + tuple = ((MinimalTupleTableSlot *) slot)->tuple;
>
> There have been various talks about the methods we could use to
> evaluate the threshold in bytes when evaluating that a flush can
> happen, including the use of memory contexts, or even estimate the
> size of the number of tuples. This one looks promising because it
> seems exact, however for virtual slots I don't like much the fact that
> you basically just extracted the parts of tts_virtual_materialize()
> and stuck them in this routine. That's a recipe for future bugs if
> the materialization logic changes. In short, I am surprised that this
> calculation is not directly part of TupleTableSlotOps. What we'd want
> is to get this information depending on the slot type dealt with, and
> with your patch you would miss to handle any new slot type
> introduced.
>
Yes for virtual slots, I reused the code from
tts_virtual_materialize() in GetTupleSize(). I can think of below
options:
1) Make the size calculation code for virtual slots, a macro or a
static inline function and use that in tts_virtual_materialize() and
GetTupleSize().
2) Add comments in both the places, such as "if any code is changed
here, consider changing it in tts_virtual_materialize() /
GetTupleSize()"
3) Add a size variable to TupleTableSlotOps structure.
4) Add a new API to TupleTableSlotOps structure say get_slot_size().
5) For new slot types, maybe we can have comments in tuptable.h to
consider having equivalent change in GetTupleSize().
If we go with 3 and 4, will it be acceptable to add the extra code in
generic structure which gets used in most of the code base and use
that new code only in limited places (for multi inserts in CTAS and
Refresh Mat View)? I think we can go ahead with 2 and 5. Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
