Re: [HACKERS] Useless code in ExecInitModifyTable

On 2018/01/15 11:28, Stephen Frost wrote:
> Fujita-san, Amit,
> 
> * Amit Langote (Langote_Amit_f8@lab.ntt.co.jp) wrote:
>> On 2017/06/21 16:59, Etsuro Fujita wrote:
>>> Commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50 added this to
>>> ExecInitModifyTable:
>>>
>>> +   /* The root table RT index is at the head of the partitioned_rels list */
>>> +   if (node->partitioned_rels)
>>> +   {
>>> +       Index   root_rti;
>>> +       Oid     root_oid;
>>> +
>>> +       root_rti = linitial_int(node->partitioned_rels);
>>> +       root_oid = getrelid(root_rti, estate->es_range_table);
>>> +       rel = heap_open(root_oid, NoLock);  /* locked by InitPlan */
>>> +   }
>>>
>>> but I noticed that that function doesn't use the relation descriptor at
>>> all.  Since partitioned_rels is given in case of an UPDATE/DELETE on a
>>> partitioned table, the relation is opened in that case, but the relation
>>> descriptor isn't referenced at all in initializing WITH CHECK OPTION
>>> constraints and/or RETURNING projections.  (The mtstate->resultRelinfo
>>> array is referenced in those initialization, instead.)  So, I'd like to
>>> propose to remove this from that function.  Attached is a patch for that.
>>
>> Thanks for cleaning that up.  I cannot see any problem in applying the patch.
> 
> Seems like this has gotten a review (and quite a bit of down-stream
> discussion that seemed pretty positive), and the latest patch still
> applies cleanly and passes the regression tests- is there some reason
> it's still marked as Needs Review?  Seems like it should really be in
> Ready for Committer state.

+1.

> Amit, if you agree, would you mind going ahead and changing it?

Sure, done.

Thanks,
Amit