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. 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/
Thanks,
Amit
