(2018/03/14 14:54), Amit Langote wrote:
On 2018/03/06 21:26, Etsuro Fujita wrote:
One thing I notice while working on this is this in ExecInsert/CopyFrom,
/*
* If we're capturing transition tuples, we might need to convert from
the
* partition rowtype to parent rowtype.
*/
if (mtstate->mt_transition_capture != NULL)
{
if (resultRelInfo->ri_TrigDesc&&
(resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
{
/*
* If there are any BEFORE or INSTEAD triggers on the partition,
* we'll have to be ready to convert their result back to
* tuplestore format.
*/
mtstate->mt_transition_capture->tcs_original_insert_tuple =
NULL;
mtstate->mt_transition_capture->tcs_map =
TupConvMapForLeaf(proute, rootRelInfo, leaf_part_index);
}
Do we need to consider INSTEAD triggers here? The partition is either a
plain table or a foreign table, so I don't think it can have those
triggers. Am I missing something?
I think you're right. We don't need to consider INSTEAD OF triggers here
if we know we're dealing with a partition which cannot have those.
Just to make sure, a word from Thomas would help.
+1
I was concerned that ExecMaterializeSlot will be called twice now -- first
in ExecPrepareTupleRouting and then in ExecInsert -- instead of only once
in ExecInsert with the existing code, but perhaps it doesn't matter much
because most of the work would be done in the 1st call anyway.
Yeah, if we force the slot into the materialized state in the 1st call,
then we won't do that in the 2nd call.
Sorry that this may be nitpicking that I should've brought up before, but
doesn't ExecPrepareTupleRouting do all the work that's needed for routing
a tuple and hence isn't the name a bit misleading? Maybe,
ExecPerformTupleRouting?
Actually, I thought of that name as a candidate, too. But I used
ExecPrepareTupleRouting because I didn't think it would actually perform
all the work; because it wouldn't do the main work of routing, ie, route
an inserted tuple to the target partition, which is ExecInsert()'s job.
I agree that it would do all the work *needed for routing*, though.
Btw, I noticed that the patches place ExecPrepareTupleRouting (both the
declaration and the definition) at different relative locations within
nodeModifyTable.c in the HEAD branch vs. in the 10 branch. It might be a
good idea to bring them to the same relative location to avoid hassle when
back-patching relevant patches in the future. I did that in the attached
updated version (v4) of the patch for HEAD, which does not make any other
changes. Although, the patch for PG-10 is unchanged, I still changed its
file name to contain v4.
That's a good idea! Thanks for the updated patches!
Best regards,
Etsuro Fujita
