Re: Slotification of partition tuple conversion

On 21 August 2018 at 08:12, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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