Re: Bogus lateral-reference-propagation logic in create_lateral_join_info

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/02/06 12:11, Tom Lane wrote:
>> I didn't run this totally to bottom yet, but what seems to be
>> happening is that inheritance_planner is creating a partition-specific
>> subplan for the DELETE, and it's allowing AppendRelInfos from the
>> parent query to propagate into the subquery even though they have
>> nothing to do with that subquery.
>> 
>> We could just decide that it's okay for code dealing with the subquery
>> to ignore the irrelevant AppendRelInfos (which is basically what my
>> draft patch did), but I find that to be an uncomfortable answer: it
>> seems *way* too likely to result in code that can mask real bugs.
>> I'd be happier fixing things so that inheritance_planner doesn't
>> propagate anything into the subquery that doesn't make sense in the
>> subquery's context.  But perhaps that's unreasonably hard?  Not enough
>> data yet.

> The target-relation specific entries in the append_rel_list of the
> original PlannerInfo are *only* for inheritance_planner to use, so it
> seems OK to ignore them during child target planning in any way possible,
> which in your patch's case is by ignoring the AppendRelInfos whose
> parent_relid fetches a NULL base rel.

I experimented with having inheritance_planner remove AppendRelInfos that
aren't relevant to the current child query.  While it's quite easy to get
rid of everything at or above the current child, as per the attached,
that's not enough to stop initsplan.c from seeing irrelevant entries:
there might still be some linking siblings of the current child to their
children, or descendants of those.  So I think we'll have to put up with
using a test-for-NULL hack in create_lateral_join_info.  I'll adjust the
comment to explain why we need it.

I'm posting the attached mainly because I'm wondering if we should
apply it despite it not being able to remove every irrelevant entry.
In many cases (particularly with wide partition trees) it'd greatly
reduce the length of the append_rel_list passed down to each
subquery, and maybe that'd save enough cycles to make it worth doing
just on performance grounds.  I've not attempted to measure though.

            regards, tom lane

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index b223972..fc0f08e 100644
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
*************** inheritance_planner(PlannerInfo *root)
*** 1307,1312 ****
--- 1307,1313 ----
          RangeTblEntry *child_rte;
          RelOptInfo *sub_final_rel;
          Path       *subpath;
+         ListCell   *lc2;

          /* append_rel_list contains all append rels; ignore others */
          if (!bms_is_member(appinfo->parent_relid, parent_relids))
*************** inheritance_planner(PlannerInfo *root)
*** 1416,1439 ****
           * want to copy items that actually contain such references; the rest
           * can just get linked into the subroot's append_rel_list.
           *
!          * If we know there are no such references, we can just use the outer
!          * append_rel_list unmodified.
           */
!         if (modifiableARIindexes != NULL)
          {
!             ListCell   *lc2;

!             subroot->append_rel_list = NIL;
!             foreach(lc2, parent_root->append_rel_list)
!             {
!                 AppendRelInfo *appinfo2 = lfirst_node(AppendRelInfo, lc2);

!                 if (bms_is_member(appinfo2->child_relid, modifiableARIindexes))
!                     appinfo2 = copyObject(appinfo2);

!                 subroot->append_rel_list = lappend(subroot->append_rel_list,
!                                                    appinfo2);
!             }
          }

          /*
--- 1417,1441 ----
           * want to copy items that actually contain such references; the rest
           * can just get linked into the subroot's append_rel_list.
           *
!          * While we're at it, drop the AppendRelInfo items that link the
!          * current parent to its children from the subquery's append_rel_list.
!          * Those items are irrelevant for the subquery, so keeping them just
!          * wastes cycles during subquery planning, and could confuse code
!          * elsewhere.
           */
!         subroot->append_rel_list = NIL;
!         foreach(lc2, parent_root->append_rel_list)
          {
!             AppendRelInfo *appinfo2 = lfirst_node(AppendRelInfo, lc2);

!             if (appinfo2->parent_relid == appinfo->parent_relid)
!                 continue;

!             if (bms_is_member(appinfo2->child_relid, modifiableARIindexes))
!                 appinfo2 = copyObject(appinfo2);

!             subroot->append_rel_list = lappend(subroot->append_rel_list,
!                                                appinfo2);
          }

          /*