From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: David Rowley <david.rowley@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Bogus lateral-reference-propagation logic in create_lateral_join_info
Date: Wed, 06 Feb 2019 11:23:22 -0500
Message-ID: <28479.1549470202@sss.pgh.pa.us> (raw)
In-Reply-To: <c8ef84f0-daa3-1fb6-8ffa-98f658a3afa5@lab.ntt.co.jp>
References: <3951.1549403812@sss.pgh.pa.us>
<CAKJS1f_j1JMQsdX+LODUywNT0WqAy6L_eF+5oZQ4FkJ_afCLMQ@mail.gmail.com>
<20001.1549422713@sss.pgh.pa.us>
<c8ef84f0-daa3-1fb6-8ffa-98f658a3afa5@lab.ntt.co.jp>
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
Attachments:
[text/x-diff] trim-append-rel-list-in-inheritance-planner.patch (2.2K, ../28479.1549470202@sss.pgh.pa.us/2-trim-append-rel-list-in-inheritance-planner.patch)
download | inline diff:diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.cindex 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);
}
/*
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, Langote_Amit_f8@lab.ntt.co.jp, david.rowley@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Bogus lateral-reference-propagation logic in create_lateral_join_info
In-Reply-To: <28479.1549470202@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox