From: Tom Lane <tgl@sss.pgh.pa.us>
To: Richard Guo <guofenglinux@gmail.com>
Cc: tharakan@gmail.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17768: Assert triggered on initsplan.c
Date: Fri, 03 Feb 2023 17:31:21 -0500
Message-ID: <1819212.1675463481@sss.pgh.pa.us> (raw)
In-Reply-To: <1600595.1675385245@sss.pgh.pa.us>
References: <17768-5ac8730ece54478f@postgresql.org>
<CAMbWs490BcsZqXF+foXiQ0zdw-FffZJA6+vBNYtJF0vkDyJ98g@mail.gmail.com>
<CAMbWs48Ggc419o1c14OLa=MtP_mgXMv=Y0h=3UcKKugCGiOQyw@mail.gmail.com>
<1600595.1675385245@sss.pgh.pa.us>
I wrote:
> I've not looked closely at this, but ... I remember thinking while
> revising deconstruct_jointree that the whole PostponedQual mechanism
> was a wart we should try to get rid of. I didn't touch it since
> I saw no obvious bugs and the patch was too large already, but maybe
> now is the time to try harder.
Here's a draft patch (no test cases as yet) that does it like that.
I think this is morally equivalent to what you did, but perhaps a
bit neater and faster.
regards, tom lane
Attachments:move-postponed-quals-into-JoinTreeItem-1.patch (text/x-diff, 19.8K) downloaddiff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.cindex 35b2dc1034..d61b827f02 100644--- a/src/backend/optimizer/plan/initsplan.c+++ b/src/backend/optimizer/plan/initsplan.c@@ -62,6 +62,8 @@ typedef struct JoinTreeItem
/* Fields filled during deconstruct_recurse: */
Node *jtnode; /* jointree node to examine */
JoinDomain *jdomain; /* join domain for its ON/WHERE clauses */
+ struct JoinTreeItem *jti_parent; /* JoinTreeItem for this node's+ * parent, or NULL if it's the top */
Relids qualscope; /* base+OJ Relids syntactically included in
* this jointree node */
Relids inner_join_rels; /* base+OJ Relids syntactically included
@@ -74,26 +76,20 @@ typedef struct JoinTreeItem
/* Fields filled during deconstruct_distribute: */
SpecialJoinInfo *sjinfo; /* if outer join, its SpecialJoinInfo */
List *oj_joinclauses; /* outer join quals not yet distributed */
+ List *lateral_clauses; /* quals postponed from children due to+ * lateral references */
} JoinTreeItem;
-/* Elements of the postponed_qual_list used during deconstruct_distribute */-typedef struct PostponedQual-{- Node *qual; /* a qual clause waiting to be processed */- Relids relids; /* the set of baserels it references */-} PostponedQual;-
static void extract_lateral_references(PlannerInfo *root, RelOptInfo *brel,
Index rtindex);
static List *deconstruct_recurse(PlannerInfo *root, Node *jtnode,
JoinDomain *parent_domain,
+ JoinTreeItem *parent_jtitem,
List **item_list);
-static void deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem,- List **postponed_qual_list);+static void deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem);
static void process_security_barrier_quals(PlannerInfo *root,
- int rti, Relids qualscope,- JoinDomain *jdomain);+ int rti, JoinTreeItem *jtitem);
static void mark_rels_nulled_by_join(PlannerInfo *root, Index ojrelid,
Relids lower_rels);
static SpecialJoinInfo *make_outerjoininfo(PlannerInfo *root,
@@ -107,7 +103,7 @@ static void deconstruct_distribute_oj_quals(PlannerInfo *root,
List *jtitems,
JoinTreeItem *jtitem);
static void distribute_quals_to_rels(PlannerInfo *root, List *clauses,
- JoinDomain *jdomain,+ JoinTreeItem *jtitem,
SpecialJoinInfo *sjinfo,
Index security_level,
Relids qualscope,
@@ -116,10 +112,9 @@ static void distribute_quals_to_rels(PlannerInfo *root, List *clauses,
bool allow_equivalence,
bool has_clone,
bool is_clone,
- List **postponed_qual_list,
List **postponed_oj_qual_list);
static void distribute_qual_to_rels(PlannerInfo *root, Node *clause,
- JoinDomain *jdomain,+ JoinTreeItem *jtitem,
SpecialJoinInfo *sjinfo,
Index security_level,
Relids qualscope,
@@ -128,7 +123,6 @@ static void distribute_qual_to_rels(PlannerInfo *root, Node *clause,
bool allow_equivalence,
bool has_clone,
bool is_clone,
- List **postponed_qual_list,
List **postponed_oj_qual_list);
static bool check_redundant_nullability_qual(PlannerInfo *root, Node *clause);
static void check_mergejoinable(RestrictInfo *restrictinfo);
@@ -742,7 +736,6 @@ deconstruct_jointree(PlannerInfo *root)
List *result;
JoinDomain *top_jdomain;
List *item_list = NIL;
- List *postponed_qual_list = NIL;
ListCell *lc;
/*
@@ -766,7 +759,7 @@ deconstruct_jointree(PlannerInfo *root)
/* Perform the initial scan of the jointree */
result = deconstruct_recurse(root, (Node *) root->parse->jointree,
- top_jdomain,+ top_jdomain, NULL,
&item_list);
/* Now we can form the value of all_query_rels, too */
@@ -780,16 +773,12 @@ deconstruct_jointree(PlannerInfo *root)
{
JoinTreeItem *jtitem = (JoinTreeItem *) lfirst(lc);
- deconstruct_distribute(root, jtitem,- &postponed_qual_list);+ deconstruct_distribute(root, jtitem);
}
- /* Shouldn't be any leftover postponed quals */- Assert(postponed_qual_list == NIL);-
/*
- * However, if there were any special joins then we may have some- * postponed LEFT JOIN clauses to deal with.+ * If there were any special joins then we may have some postponed LEFT+ * JOIN clauses to deal with.
*/
if (root->join_info_list)
{
@@ -814,7 +803,8 @@ deconstruct_jointree(PlannerInfo *root)
*
* jtnode is the jointree node to examine, and parent_domain is the
* enclosing join domain. (We must add all base+OJ relids appearing
- * here or below to parent_domain.)+ * here or below to parent_domain.) parent_jtitem is the JoinTreeItem+ * for the parent jointree node, or NULL at the top of the recursion.
*
* item_list is an in/out parameter: we add a JoinTreeItem struct to
* that list for each jointree node, in depth-first traversal order.
@@ -825,6 +815,7 @@ deconstruct_jointree(PlannerInfo *root)
static List *
deconstruct_recurse(PlannerInfo *root, Node *jtnode,
JoinDomain *parent_domain,
+ JoinTreeItem *parent_jtitem,
List **item_list)
{
List *joinlist;
@@ -835,6 +826,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
/* Make the new JoinTreeItem, but don't add it to item_list yet */
jtitem = palloc0_object(JoinTreeItem);
jtitem->jtnode = jtnode;
+ jtitem->jti_parent = parent_jtitem;
if (IsA(jtnode, RangeTblRef))
{
@@ -880,6 +872,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
sub_joinlist = deconstruct_recurse(root, lfirst(l),
parent_domain,
+ jtitem,
item_list);
sub_item = (JoinTreeItem *) llast(*item_list);
jtitem->qualscope = bms_add_members(jtitem->qualscope,
@@ -922,10 +915,12 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
/* Recurse */
leftjoinlist = deconstruct_recurse(root, j->larg,
parent_domain,
+ jtitem,
item_list);
left_item = (JoinTreeItem *) llast(*item_list);
rightjoinlist = deconstruct_recurse(root, j->rarg,
parent_domain,
+ jtitem,
item_list);
right_item = (JoinTreeItem *) llast(*item_list);
/* Compute qualscope etc */
@@ -947,10 +942,12 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
/* Recurse */
leftjoinlist = deconstruct_recurse(root, j->larg,
parent_domain,
+ jtitem,
item_list);
left_item = (JoinTreeItem *) llast(*item_list);
rightjoinlist = deconstruct_recurse(root, j->rarg,
child_domain,
+ jtitem,
item_list);
right_item = (JoinTreeItem *) llast(*item_list);
/* Compute join domain contents, qualscope etc */
@@ -984,10 +981,12 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
/* Recurse */
leftjoinlist = deconstruct_recurse(root, j->larg,
parent_domain,
+ jtitem,
item_list);
left_item = (JoinTreeItem *) llast(*item_list);
rightjoinlist = deconstruct_recurse(root, j->rarg,
parent_domain,
+ jtitem,
item_list);
right_item = (JoinTreeItem *) llast(*item_list);
/* Compute qualscope etc */
@@ -1013,6 +1012,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
root->join_domains = lappend(root->join_domains, child_domain);
leftjoinlist = deconstruct_recurse(root, j->larg,
child_domain,
+ jtitem,
item_list);
left_item = (JoinTreeItem *) llast(*item_list);
fj_domain->jd_relids = bms_copy(child_domain->jd_relids);
@@ -1021,6 +1021,7 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
root->join_domains = lappend(root->join_domains, child_domain);
rightjoinlist = deconstruct_recurse(root, j->rarg,
child_domain,
+ jtitem,
item_list);
right_item = (JoinTreeItem *) llast(*item_list);
/* Compute qualscope etc */
@@ -1108,20 +1109,9 @@ deconstruct_recurse(PlannerInfo *root, Node *jtnode,
*
* Distribute quals of the node to appropriate restriction and join lists.
* In addition, entries will be added to root->join_info_list for outer joins.
- *- * Inputs:- * jtitem is the JoinTreeItem to examine- * Input/Outputs:- * *postponed_qual_list is a list of PostponedQual structs- *- * On entry, *postponed_qual_list contains any quals that had to be postponed- * out of lower join levels (because they contain lateral references).- * On exit, *postponed_qual_list contains quals that can't be processed yet- * (because their lateral references are still unsatisfied).
*/
static void
-deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem,- List **postponed_qual_list)+deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem)
{
Node *jtnode = jtitem->jtnode;
@@ -1133,82 +1123,51 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem,
if (root->qual_security_level > 0)
process_security_barrier_quals(root,
varno,
- jtitem->qualscope,- jtitem->jdomain);+ jtitem);
}
else if (IsA(jtnode, FromExpr))
{
FromExpr *f = (FromExpr *) jtnode;
- List *new_postponed_quals = NIL;- ListCell *l;
/*
- * Try to process any quals postponed by children. If they need- * further postponement, add them to my output postponed_qual_list.+ * Process any lateral-referencing quals that were postponed to this+ * level by children.
*/
- foreach(l, *postponed_qual_list)- {- PostponedQual *pq = (PostponedQual *) lfirst(l);-- if (bms_is_subset(pq->relids, jtitem->qualscope))- distribute_qual_to_rels(root, pq->qual,- jtitem->jdomain,- NULL,- root->qual_security_level,- jtitem->qualscope, NULL, NULL,- true, false, false,- NULL, NULL);- else- new_postponed_quals = lappend(new_postponed_quals, pq);- }- *postponed_qual_list = new_postponed_quals;+ distribute_quals_to_rels(root, jtitem->lateral_clauses,+ jtitem,+ NULL,+ root->qual_security_level,+ jtitem->qualscope, NULL, NULL,+ true, false, false,+ NULL);
/*
* Now process the top-level quals.
*/
distribute_quals_to_rels(root, (List *) f->quals,
- jtitem->jdomain,+ jtitem,
NULL,
root->qual_security_level,
jtitem->qualscope, NULL, NULL,
true, false, false,
- postponed_qual_list, NULL);+ NULL);
}
else if (IsA(jtnode, JoinExpr))
{
JoinExpr *j = (JoinExpr *) jtnode;
- List *new_postponed_quals = NIL;
Relids ojscope;
List *my_quals;
SpecialJoinInfo *sjinfo;
List **postponed_oj_qual_list;
- ListCell *l;
/*
- * Try to process any quals postponed by children. If they need- * further postponement, add them to my output postponed_qual_list.- * Quals that can be processed now must be included in my_quals, so- * that they'll be handled properly in make_outerjoininfo.+ * Include lateral-referencing quals postponed from children in+ * my_quals, so that they'll be handled properly in+ * make_outerjoininfo. (This is destructive to+ * jtitem->lateral_clauses, but we won't use that again.)
*/
- my_quals = NIL;- foreach(l, *postponed_qual_list)- {- PostponedQual *pq = (PostponedQual *) lfirst(l);-- if (bms_is_subset(pq->relids, jtitem->qualscope))- my_quals = lappend(my_quals, pq->qual);- else- {- /*- * We should not be postponing any quals past an outer join.- * If this Assert fires, pull_up_subqueries() messed up.- */- Assert(j->jointype == JOIN_INNER);- new_postponed_quals = lappend(new_postponed_quals, pq);- }- }- *postponed_qual_list = new_postponed_quals;- my_quals = list_concat(my_quals, (List *) j->quals);+ my_quals = list_concat(jtitem->lateral_clauses,+ (List *) j->quals);
/*
* For an OJ, form the SpecialJoinInfo now, so that we can pass it to
@@ -1268,14 +1227,13 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem,
/* Process the JOIN's qual clauses */
distribute_quals_to_rels(root, my_quals,
- jtitem->jdomain,+ jtitem,
sjinfo,
root->qual_security_level,
jtitem->qualscope,
ojscope, jtitem->nonnullable_rels,
true, /* allow_equivalence */
false, false, /* not clones */
- postponed_qual_list,
postponed_oj_qual_list);
/* And add the SpecialJoinInfo to join_info_list */
@@ -1304,8 +1262,7 @@ deconstruct_distribute(PlannerInfo *root, JoinTreeItem *jtitem,
*/
static void
process_security_barrier_quals(PlannerInfo *root,
- int rti, Relids qualscope,- JoinDomain *jdomain)+ int rti, JoinTreeItem *jtitem)
{
RangeTblEntry *rte = root->simple_rte_array[rti];
Index security_level = 0;
@@ -1328,15 +1285,14 @@ process_security_barrier_quals(PlannerInfo *root,
* pushed up to top of tree, which we don't want.
*/
distribute_quals_to_rels(root, qualset,
- jdomain,+ jtitem,
NULL,
security_level,
- qualscope,- qualscope,+ jtitem->qualscope,+ jtitem->qualscope,
NULL,
true,
false, false, /* not clones */
- NULL,
NULL);
security_level++;
}
@@ -2038,7 +1994,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
is_clone = !has_clone;
distribute_quals_to_rels(root, quals,
- otherjtitem->jdomain,+ otherjtitem,
sjinfo,
root->qual_security_level,
this_qualscope,
@@ -2046,7 +2002,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
allow_equivalence,
has_clone,
is_clone,
- NULL, NULL); /* no more postponement */+ NULL); /* no more postponement */
/*
* Adjust qual nulling bits for next level up, if needed. We
@@ -2067,14 +2023,14 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
{
/* No commutation possible, just process the postponed clauses */
distribute_quals_to_rels(root, jtitem->oj_joinclauses,
- jtitem->jdomain,+ jtitem,
sjinfo,
root->qual_security_level,
qualscope,
ojscope, nonnullable_rels,
true, /* allow_equivalence */
false, false, /* not clones */
- NULL, NULL); /* no more postponement */+ NULL); /* no more postponement */
}
}
@@ -2092,7 +2048,7 @@ deconstruct_distribute_oj_quals(PlannerInfo *root,
*/
static void
distribute_quals_to_rels(PlannerInfo *root, List *clauses,
- JoinDomain *jdomain,+ JoinTreeItem *jtitem,
SpecialJoinInfo *sjinfo,
Index security_level,
Relids qualscope,
@@ -2101,7 +2057,6 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
bool allow_equivalence,
bool has_clone,
bool is_clone,
- List **postponed_qual_list,
List **postponed_oj_qual_list)
{
ListCell *lc;
@@ -2111,7 +2066,7 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
Node *clause = (Node *) lfirst(lc);
distribute_qual_to_rels(root, clause,
- jdomain,+ jtitem,
sjinfo,
security_level,
qualscope,
@@ -2120,7 +2075,6 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
allow_equivalence,
has_clone,
is_clone,
- postponed_qual_list,
postponed_oj_qual_list);
}
}
@@ -2134,12 +2088,12 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
* mergejoinable operator, enter its left- and right-side expressions into
* the query's EquivalenceClasses.
*
- * In some cases, quals will be added to postponed_qual_list or+ * In some cases, quals will be added to parent jtitems' lateral_clauses or
* postponed_oj_qual_list instead of being processed right away.
* These will be dealt with in later steps of deconstruct_jointree.
*
* 'clause': the qual clause to be distributed
- * 'jdomain': the join domain containing the clause+ * 'jtitem': the JoinTreeItem for the containing jointree node
* 'sjinfo': join's SpecialJoinInfo (NULL for an inner join or WHERE clause)
* 'security_level': security_level to assign to the qual
* 'qualscope': set of base+OJ rels the qual's syntactic scope covers
@@ -2153,9 +2107,6 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
* EquivalenceClass
* 'has_clone': has_clone property to assign to the qual
* 'is_clone': is_clone property to assign to the qual
- * 'postponed_qual_list': list of PostponedQual structs, which we can add- * this qual to if it turns out to belong to a higher join level.- * Can be NULL if caller knows postponement is impossible.
* 'postponed_oj_qual_list': if not NULL, non-degenerate outer join clauses
* should be added to this list instead of being processed (list entries
* are just the bare clauses)
@@ -2170,7 +2121,7 @@ distribute_quals_to_rels(PlannerInfo *root, List *clauses,
*/
static void
distribute_qual_to_rels(PlannerInfo *root, Node *clause,
- JoinDomain *jdomain,+ JoinTreeItem *jtitem,
SpecialJoinInfo *sjinfo,
Index security_level,
Relids qualscope,
@@ -2179,7 +2130,6 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
bool allow_equivalence,
bool has_clone,
bool is_clone,
- List **postponed_qual_list,
List **postponed_oj_qual_list)
{
Relids relids;
@@ -2202,19 +2152,32 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
* level that includes every rel they reference. Although we could make
* pull_up_subqueries() place such quals correctly to begin with, it's
* easier to handle it here. When we find a clause that contains Vars
- * outside its syntactic scope, we add it to the postponed-quals list, and- * process it once we've recursed back up to the appropriate join level.+ * outside its syntactic scope, locate the nearest parent join level that+ * includes all the required rels and add the clause to that level's+ * lateral_clauses list. We'll process it when we reach that join level.
*/
if (!bms_is_subset(relids, qualscope))
{
- PostponedQual *pq = (PostponedQual *) palloc(sizeof(PostponedQual));+ JoinTreeItem *pitem;
Assert(root->hasLateralRTEs); /* shouldn't happen otherwise */
Assert(sjinfo == NULL); /* mustn't postpone past outer join */
- pq->qual = clause;- pq->relids = relids;- *postponed_qual_list = lappend(*postponed_qual_list, pq);- return;+ for (pitem = jtitem->jti_parent; pitem; pitem = pitem->jti_parent)+ {+ if (bms_is_subset(relids, pitem->qualscope))+ {+ pitem->lateral_clauses = lappend(pitem->lateral_clauses,+ clause);+ return;+ }++ /*+ * We should not be postponing any quals past an outer join. If+ * this Assert fires, pull_up_subqueries() messed up.+ */+ Assert(pitem->sjinfo == NULL);+ }+ elog(ERROR, "failed to postpone qual containing lateral reference");
}
/*
@@ -2262,7 +2225,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
else
{
/* eval at join domain level */
- relids = bms_copy(jdomain->jd_relids);+ relids = bms_copy(jtitem->jdomain->jd_relids);
/* mark as gating qual */
pseudoconstant = true;
/* tell createplan.c to check for gating quals */
@@ -2458,7 +2421,7 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
{
if (maybe_equivalence)
{
- if (process_equivalence(root, &restrictinfo, jdomain))+ if (process_equivalence(root, &restrictinfo, jtitem->jdomain))
return;
/* EC rejected it, so set left_ec/right_ec the hard way ... */
if (restrictinfo->mergeopfamilies) /* EC might have changed this */
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-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, guofenglinux@gmail.com, tharakan@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17768: Assert triggered on initsplan.c
In-Reply-To: <1819212.1675463481@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