Re: inserts into partitioned table may cause crash

(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