Re: Useless code in ExecInitModifyTable

On Wed, Jun 21, 2017 at 10:33 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> writes:
>> On 2017/06/21 16:59, Etsuro Fujita wrote:
>>> 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.
>
> Actually, isn't ModifyTable.partitioned_rels *always* NIL? I can't
> see anyplace in the planner that sets it differently. I think we should
> flush the field altogether. Anybody who doesn't want that should at least
> provide the missing comment for create_modifytable_path's partitioned_rels
> argument (and yes, the fact that that is missing isn't making me any
> more favorably disposed...)

It appears to me that create_modifytable_path() has a partitioned_rels
argument and it looks like inheritance_planner not only passes it but
guarantees that it's non-empty when the target is a partitioned table.
You're right that there is a comment missing from the function header,
but it's not too hard to figure out what it should be. Just consult
the definition of ModifyTable itself:

/* RT indexes of non-leaf tables in a partition tree */
List *partitioned_rels;

The point here is that if we have a partitioned table with a bunch of
descendent tables, the non-leaf partitions are never actually scanned;
there's no file on disk to scan. Despite the lack of a scan, we still
need to arrange for those relations to be locked.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company