Re: inserts into partitioned table may cause crash

Fujita-san,

Thanks for the updates and sorry I couldn't reply sooner.

On 2018/03/06 21:26, Etsuro Fujita wrote:
> One thing I notice while working on this is this in ExecInsert/CopyFrom,
> which I moved to ExecPrepareTupleRouting as-is for the former:
>
>     /*
>      * 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.

On 2018/03/12 12:25, Etsuro Fujita wrote:
> (2018/03/09 20:18), Etsuro Fujita wrote:
>> Here are updated patches for PG10 and HEAD.
>>
>> Other changes:
>> * Add regression tests based on your test cases shown upthread
> 
> I added a little bit more regression tests and revised comments.  Please
> find attached an updated patch.

Thanks for adding the test.

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.

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?

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.

Regards,
Amit