Re: Slotification of partition tuple conversion

On 3 September 2018 at 12:14, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> Hi Amit,
>
> Thanks for the updated patch and sorry I couldn't reply sooner.
>
> On 2018/08/21 16:18, Amit Khandekar wrote:
>> 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.
>
> Hmm, okay.
>
> The verb here is "to convert", which still applies to a tuple, the only
> difference is that the new interface accepts and returns TupleTableSlot,
> instead of HeapTuple.

Yeah, in that sense you are right. Let's see others' suggestions. For
now I haven't changed it.

> @@ -2691,12 +2861,14 @@ CopyFrom(CopyState cstate)
>
> /*
> * We might need to convert from the parent rowtype to the
> - * partition rowtype.
> + * partition rowtype. Don't free the already stored tuple as it
> + * may still be required for a multi-insert batch.
> */
> tuple =
> ConvertPartitionTupleSlot(proute->parent_child_tupconv_maps[leaf_part_index],
> tuple,
> proute->partition_tuple_slot,
> - &slot);
> + &slot,
> + false);
>
> So the "already stored tuple" means a previous tuple that may have been
> stored in 'proute->partition_tuple_slot', which shouldn't be freed in
> ConvertPartitionTupleSlot (via ExecStoreTuple), because it might also have
> been stored in the batch insert tuple array.
>
> Now, because with ConvertTupleSlot, there is no worry about freeing an
> existing tuple, because we never perform ExecStoreTuple on
> proute->partition_tuple_slot (or one of the slots in it if we consider my
> patch at [1] which converts it to an array). All I see applied to those
> slots is ExecStoreVirtualTuple() in ConvertTupleSlot(), and in some cases,
> ExecCopySlotTuple() in the caller of ConvertTupleSlot().
>
> So, I think that that sentence is obsolete with your patch.

Right. Removed the "Don't free the already stored tuple" part.

> I was saying, because we no longer use do_convert_tuple for
> "partitioning-related" tuple conversions, we could get rid of
> TupleConversionMaps in the partitioning-specific PartitionTupleRouting
> structure.

We use child_parent_tupconv_maps in AfterTriggerSaveEvent() where we
call do_convert_tuple() with the map.
We pass parent_child_tupconv_maps to adjust_partition_tlist(), which
requires the map->outdesc.
So there are these places where still we can't get rid of
TupleConversionMaps even for partition-related tuples.

Regarding adjust_partition_tlist(), we can perhaps pass the outdesc
from somewhere else rather than map->outdesc, making it possible to
use AttrNumber map rather than TupleConversionMap. But it needs some
investigation. Overall, I think both the above maps should be worked
on as a separate item, and not put in this patch that focusses on
ConvertTupleSlot().

I have changed the PartitionDispatch->tupmap type to AttrNumber[],
though. This was possible because we are using the tupmap->attrMap and
no other fields.

>
> Also, since ConvertTupleSlot itself just uses the attrMap field of
> TupleConversionMap, I was wondering if its signature should contain
> AttrNumber * instead of TupleConversionMap *?

Done.

>
> Maybe if we get around to getting rid of do_convert_tuple altogether, that
> would also mean getting rid of the TupleConversionMap struct, because its
> various tuple data related arrays would be redundant, because
> ConvertTupleSlot has input and output TupleTableSlots, which have space
> for that information.

Yeah agree. We will be working towards that.

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