On 2018/08/20 20:15, Amit Khandekar wrote:
> On 17 August 2018 at 21:47, Amit Khandekar <amitdkhan.pg@gmail.com> wrote:
>
>> Attached is a patch tup_convert.patch that does the conversion
>> directly using input and output tuple slots. This patch is to be
>> applied on an essential patch posted by Amit Langote in [1] that
>> arranges for dedicated per-partition slots rather than changing
>> tupdesc of a single slot. I have rebased that patch from [1] and
>> attached it here (
>> Allocate-dedicated-slots-of-partition-tuple_amitlan_rebased.patch).
>
> Amit Langote has posted a new version of the
> Allocate-dedicated-slots-of-partition-tuple_amitlan_rebased.patch at
> [1] . Attached is version v2 of the tup_convert.patch, that is rebased
> over that patch.
Thanks.
Here are some comments on the patch:
+ConvertTupleSlot
Might be a good idea to call this ConvertSlotTuple?
+ /*
+ * 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)?
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.
Also, how about putting ConvertTupleSlot in execPartition.c and exporting
it in execPartition.h, instead of tupconvert.c and tupconvert.h, resp.?
Thanks,
Amit
