On 20 August 2018 at 14:45, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the review.
>
> On 2018/08/17 15:00, Amit Khandekar wrote:
>> Thanks for the patch. I did some review of the patch (needs a rebase).
>> Below are my comments.
>>
>> @@ -1936,12 +1936,11 @@ ExecPartitionCheckEmitError(ResultRelInfo
>> *resultRelInfo,
>> + /* Input slot might be of a partition, which has a fixed tupdesc. */
>> + slot = MakeTupleTableSlot(tupdesc);
>> if (map != NULL)
>> - {
>> tuple = do_convert_tuple(tuple, map);
>> - ExecSetSlotDescriptor(slot, tupdesc);
>> - ExecStoreTuple(tuple, slot, InvalidBuffer, false);
>> - }
>> + ExecStoreTuple(tuple, slot, InvalidBuffer, false);
>>
>> Both MakeTupleTableSlot() and ExecStoreTuple() can be inside the (map
>> != NULL) if condition.
>> This also applies for similar changes in ExecConstraints() and
>> ExecWithCheckOptions().
>
> Ah, okay. I guess that means we'll allocate a new slot here only if we
> had to switch to a partition-specific slot in the first place.
>
>> + * Initialize an empty slot that will be used to manipulate tuples of any
>> + * this partition's rowtype.
>> of any this => of this
>>
>> + * Initialized the array where these slots are stored, if not already
>> Initialized => Initialize
>
> Fixed.
>
>> + proute->partition_tuple_slots_alloced =
>> + lappend(proute->partition_tuple_slots_alloced,
>> + proute->partition_tuple_slots[partidx]);
>>
>> Instead of doing the above, I think we can collect those slots in
>> estate->es_tupleTable using ExecInitExtraTupleSlot() so that they
>> don't have to be explicitly dropped in ExecCleanupTupleRouting(). And
>> also then we won't require the new field
>> proute->partition_tuple_slots_alloced.
>
> Although I was slightly uncomfortable of the idea at first, thinking that
> it's not good for tuple routing specific resources to be released by
> generic executor code, doesn't seem too bad to do it the way you suggest.
>
> Attached updated patch.
Thanks for the changes. The patch looks good to me.
> By the way, I think it might be a good idea to
> try to merge this patch with the effort in the following thread.
>
> * Reduce partition tuple routing overheads *
> https://commitfest.postgresql.org/19/1690/
Yes. I guess, whichever commits later needs to be rebased on the earlier one.
--
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company
