Re: Partition-wise join for join between (declaratively) partitioned tables

On Tue, Sep 12, 2017 at 2:35 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2017/09/12 17:53, Ashutosh Bapat wrote:
>> On Tue, Sep 12, 2017 at 1:42 PM, Amit Langote wrote:
>>> So, we can remove partitioned_rels from (Merge)AppendPath and
>>> (Merge)Append nodes and remove ExecLockNonLeafAppendTables().
>>
>> Don't we need partitioned_rels from Append paths to be transferred to
>> ModifyTable node or we have a different way of calculating
>> nonleafResultRelations?
>
> No, we don't transfer partitioned_rels from Append path to ModifyTable
> node.  inheritance_planner(), that builds the ModifyTable path for
> UPDATE/DELETE on a partitioned table, fetches partitioned_rels from
> root->pcinfo_list itself and passes it to create_modifytable_path.  No
> Append path is involved in that case.  PlannedStmt.nonleafResultRelations
> is built by concatenating the partitioned_rels lists of all ModifyTable
> nodes appearing in the query.  It does not depend on Append's or
> AppendPath's partitioned_rels.

Ok. Thanks for the explanation.

This make me examine inheritance_planner() closely and I think I have
spotted a thinko there. In inheritance_planner() parent_rte is set to
the RTE of parent to start with and then in the loop
1132     /*
1133      * And now we can get on with generating a plan for each child table.
1134      */
1135     foreach(lc, root->append_rel_list)
1136     {
... code clipped
1165         /*
1166          * If there are securityQuals attached to the parent,
move them to the
1167          * child rel (they've already been transformed properly for that).
1168          */
1169         parent_rte = rt_fetch(parentRTindex, subroot->parse->rtable);
1170         child_rte = rt_fetch(appinfo->child_relid, subroot->parse->rtable);
1171         child_rte->securityQuals = parent_rte->securityQuals;
1172         parent_rte->securityQuals = NIL;

we set parent_rte to the one obtained from subroot->parse, which
happens to be the same (at least in contents) as original parent_rte.
Later we use this parent_rte to pull partitioned_rels outside that
loop

1371     if (parent_rte->relkind == RELKIND_PARTITIONED_TABLE)
1372     {
1373         partitioned_rels = get_partitioned_child_rels(root, parentRTindex);
1374         /* The root partitioned table is included as a child rel */
1375         Assert(list_length(partitioned_rels) >= 1);
1376     }

I think the code here expects the original parent_rte and not the one
we set around line 1169.

This isn't a bug right now, since both the parent_rte s have same
content. But I am not sure if that will remain to be so. Here's patch
to fix the thinko.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company