Re: Slotification of partition tuple conversion

On 21 August 2018 at 08:12, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Here are some comments on the patch:

Thanks for the review.

>
> +ConvertTupleSlot
>
> Might be a good idea to call this ConvertSlotTuple?

I thought the name 'ConvertTupleSlot' emphasizes the fact that we are
operating rather on a slot without having to touch the tuple wherever
possible. Hence I chose the name. But I am open to suggestions if
there are more votes against this.

>
> +                /*
> +                 * Get the tuple in the per-tuple context. Also, we
> cannot call
> +                 * ExecMaterializeSlot(), otherwise the tuple will get freed
> +                 * while storing the next tuple.
> +                 */
> +                oldcontext =
> MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
> +                tuple = ExecCopySlotTuple(slot);
> +                MemoryContextSwitchTo(oldcontext);
>
> The comment about why we cannot call ExecMaterializeSlot is a bit unclear.
>  Maybe, the comment doesn't need to mention that?  Instead, expand a bit
> more on why the context switch here or how it interacts with recently
> tuple buffering (0d5f05cde01)?

Done :

-   * Get the tuple in the per-tuple context. Also, we cannot call
-   * ExecMaterializeSlot(), otherwise the tuple will get freed
-   * while storing the next tuple.
+  * Get the tuple in the per-tuple context, so that it will be
+  * freed after each batch insert.

Specifically, we could not call ExecMaterializeSlot() because it sets
tts_shouldFree to true which we don't want for batch inserts.

>
> Seeing that all the sites in the partitioning code that previously called
> do_convert_tuple() now call ConvertTupleSlot, I wonder if we don't need a
> full TupleConversionMap, just the attribute map in it.  We don't need the
> input/output Datum and bool arrays in it, because we'd be using the ones
> from input and output slots of ConvertTupleSlot.  So, can we replace all
> instances of TupleConversionMap in the partitioning code and data
> structures by AttributeNumber arrays.

Yeah, I earlier thought I could get rid of do_convert_tuple()
altogether. But there are places where we currently deal with tuples
rather than slots. And here, we could not replace do_convert_tuple()
unless we slotify the surrounding code similar to how you did in the
Allocate-dedicated-slots patch. E.g.  ExecEvalConvertRowtype() and
AfterTriggerSaveEvent() deals with tuples so the do_convert_tuple()
calls there couldn't be replaced.

So I think we can work towards what you have in mind in form of
incremental patches.

>
> Also, how about putting ConvertTupleSlot in execPartition.c and exporting
> it in execPartition.h, instead of tupconvert.c and tupconvert.h, resp.?

I think the goal of ConvertTupleSlot() is to eventually replace
do_convert_tuple() as well, so it would look more of a general
conversion rather than specific for partitions. Hence I think it's
better to have it in tupconvert.c .



-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company