Re: Costing elided SubqueryScans more nearly correctly
Alvaro Herrera writes: > Thanks, pushed. Pushed the original patch now too. regards, tom lane
Re: Costing elided SubqueryScans more nearly correctly
On 2022-Jul-19, Richard Guo wrote: > On Tue, Jul 19, 2022 at 1:30 AM Tom Lane wrote: > > WFM. (I'd fixed the comment typo in my patch, but I don't mind if > > you get there first.) Ah, I see now you had other grammatical fixes and even more content there. > +1 The fix looks good to me. Thanks, pushed. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Costing elided SubqueryScans more nearly correctly
On Tue, Jul 19, 2022 at 1:30 AM Tom Lane wrote: > Alvaro Herrera writes: > > On 2022-Jul-18, Richard Guo wrote: > >> BTW, not related to this patch, the new lines for parallel_aware check > >> in setrefs.c are very wide. How about wrap them to keep consistent with > >> arounding codes? > > > Not untrue! Something like this, you mean? Fixed the nearby typo while > > at it. > > WFM. (I'd fixed the comment typo in my patch, but I don't mind if > you get there first.) +1 The fix looks good to me. Thanks Richard
Re: Costing elided SubqueryScans more nearly correctly
Alvaro Herrera writes: > On 2022-Jul-18, Richard Guo wrote: >> BTW, not related to this patch, the new lines for parallel_aware check >> in setrefs.c are very wide. How about wrap them to keep consistent with >> arounding codes? > Not untrue! Something like this, you mean? Fixed the nearby typo while > at it. WFM. (I'd fixed the comment typo in my patch, but I don't mind if you get there first.) regards, tom lane
Re: Costing elided SubqueryScans more nearly correctly
On 2022-Jul-18, Richard Guo wrote:
> BTW, not related to this patch, the new lines for parallel_aware check
> in setrefs.c are very wide. How about wrap them to keep consistent with
> arounding codes?
Not untrue! Something like this, you mean? Fixed the nearby typo while
at it.
--
Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
>From 985ffec3086fc01585cb784a58ddb8975f832350 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 18 Jul 2022 16:40:01 +0200
Subject: [PATCH] Wrap overly long lines
---
src/backend/optimizer/plan/setrefs.c | 29
1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 9cef92cab2..707c1016c2 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1637,14 +1637,16 @@ set_append_references(PlannerInfo *root,
* See if it's safe to get rid of the Append entirely. For this to be
* safe, there must be only one child plan and that child plan's parallel
* awareness must match that of the Append's. The reason for the latter
- * is that the if the Append is parallel aware and the child is not then
- * the calling plan may execute the non-parallel aware child multiple
- * times.
+ * is that if the Append is parallel aware and the child is not, then the
+ * calling plan may execute the non-parallel aware child multiple times.
*/
- if (list_length(aplan->appendplans) == 1 &&
- ((Plan *) linitial(aplan->appendplans))->parallel_aware == aplan->plan.parallel_aware)
- return clean_up_removed_plan_level((Plan *) aplan,
- (Plan *) linitial(aplan->appendplans));
+ if (list_length(aplan->appendplans) == 1)
+ {
+ Plan *p = (Plan *) linitial(aplan->appendplans);
+
+ if (p->parallel_aware == aplan->plan.parallel_aware)
+ return clean_up_removed_plan_level((Plan *) aplan, p);
+ }
/*
* Otherwise, clean up the Append as needed. It's okay to do this after
@@ -1709,14 +1711,17 @@ set_mergeappend_references(PlannerInfo *root,
* See if it's safe to get rid of the MergeAppend entirely. For this to
* be safe, there must be only one child plan and that child plan's
* parallel awareness must match that of the MergeAppend's. The reason
- * for the latter is that the if the MergeAppend is parallel aware and the
+ * for the latter is that if the MergeAppend is parallel aware and the
* child is not then the calling plan may execute the non-parallel aware
* child multiple times.
*/
- if (list_length(mplan->mergeplans) == 1 &&
- ((Plan *) linitial(mplan->mergeplans))->parallel_aware == mplan->plan.parallel_aware)
- return clean_up_removed_plan_level((Plan *) mplan,
- (Plan *) linitial(mplan->mergeplans));
+ if (list_length(mplan->mergeplans) == 1)
+ {
+ Plan *p = (Plan *) linitial(mplan->mergeplans);
+
+ if (p->parallel_aware == mplan->plan.parallel_aware)
+ return clean_up_removed_plan_level((Plan *) mplan, p);
+ }
/*
* Otherwise, clean up the MergeAppend as needed. It's okay to do this
--
2.30.2
Re: Costing elided SubqueryScans more nearly correctly
On Mon, Jul 18, 2022 at 3:16 AM Tom Lane wrote: > I wrote: > > I also notice that setrefs.c can elide Append and MergeAppend nodes > > too in some cases, but AFAICS costing of those node types doesn't > > take that into account. > > I took a closer look at this point and realized that in fact, > create_append_path and create_merge_append_path do attempt to account > for this. But they get it wrong! Somebody changed the rules in > setrefs.c to account for parallelism, and did not update the costing > side of things. > > The attached v2 is the same as v1 plus adding a fix for this point. > No regression test results change from that AFAICT. The new fix looks good to me. Seems setrefs.c added a new logic to check parallel_aware when removing single-child Appends/MergeAppends in f9a74c14, but it neglected to update the related costing logic. And I can see this patch fixes the costing for that. BTW, not related to this patch, the new lines for parallel_aware check in setrefs.c are very wide. How about wrap them to keep consistent with arounding codes? Thanks Richard
Re: Costing elided SubqueryScans more nearly correctly
I wrote:
> I also notice that setrefs.c can elide Append and MergeAppend nodes
> too in some cases, but AFAICS costing of those node types doesn't
> take that into account.
I took a closer look at this point and realized that in fact,
create_append_path and create_merge_append_path do attempt to account
for this. But they get it wrong! Somebody changed the rules in
setrefs.c to account for parallelism, and did not update the costing
side of things.
The attached v2 is the same as v1 plus adding a fix for this point.
No regression test results change from that AFAICT.
regards, tom lane
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index 358bb2aed6..028d9e1680 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2451,6 +2451,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
{
Query *parse = root->parse;
Query *subquery = rte->subquery;
+ bool trivial_pathtarget;
Relids required_outer;
pushdown_safety_info safetyInfo;
double tuple_fraction;
@@ -2613,6 +2614,36 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
*/
set_subquery_size_estimates(root, rel);
+ /*
+ * Also detect whether the reltarget is trivial, so that we can pass that
+ * info to cost_subqueryscan (rather than re-deriving it multiple times).
+ * It's trivial if it fetches all the subplan output columns in order.
+ */
+ if (list_length(rel->reltarget->exprs) != list_length(subquery->targetList))
+ trivial_pathtarget = false;
+ else
+ {
+ trivial_pathtarget = true;
+ foreach(lc, rel->reltarget->exprs)
+ {
+ Node *node = (Node *) lfirst(lc);
+ Var *var;
+
+ if (!IsA(node, Var))
+ {
+trivial_pathtarget = false;
+break;
+ }
+ var = (Var *) node;
+ if (var->varno != rti ||
+var->varattno != foreach_current_index(lc) + 1)
+ {
+trivial_pathtarget = false;
+break;
+ }
+ }
+ }
+
/*
* For each Path that subquery_planner produced, make a SubqueryScanPath
* in the outer query.
@@ -2631,6 +2662,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
/* Generate outer path using this subpath */
add_path(rel, (Path *)
create_subqueryscan_path(root, rel, subpath,
+ trivial_pathtarget,
pathkeys, required_outer));
}
@@ -2656,6 +2688,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
/* Generate outer path using this subpath */
add_partial_path(rel, (Path *)
create_subqueryscan_path(root, rel, subpath,
+ trivial_pathtarget,
pathkeys,
required_outer));
}
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 5e5732f6e1..fb28e6411a 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1415,10 +1415,12 @@ cost_tidrangescan(Path *path, PlannerInfo *root,
*
* 'baserel' is the relation to be scanned
* 'param_info' is the ParamPathInfo if this is a parameterized path, else NULL
+ * 'trivial_pathtarget' is true if the pathtarget is believed to be trivial.
*/
void
cost_subqueryscan(SubqueryScanPath *path, PlannerInfo *root,
- RelOptInfo *baserel, ParamPathInfo *param_info)
+ RelOptInfo *baserel, ParamPathInfo *param_info,
+ bool trivial_pathtarget)
{
Cost startup_cost;
Cost run_cost;
@@ -1458,6 +1460,22 @@ cost_subqueryscan(SubqueryScanPath *path, PlannerInfo *root,
path->path.startup_cost = path->subpath->startup_cost;
path->path.total_cost = path->subpath->total_cost;
+ /*
+ * However, if there are no relevant restriction clauses and the
+ * pathtarget is trivial, then we expect that setrefs.c will optimize away
+ * the SubqueryScan plan node altogether, so we should just make its cost
+ * and rowcount equal to the input path's.
+ *
+ * Note: there are some edge cases where createplan.c will apply a
+ * different targetlist to the SubqueryScan node, thus falsifying our
+ * current estimate of whether the target is trivial, and making the cost
+ * estimate (though not the rowcount) wrong. It does not seem worth the
+ * extra complication to try to account for that exactly, especially since
+ * that behavior falsifies other cost estimates as well.
+ */
+ if (qpquals == NIL && trivial_pathtarget)
+ return;
+
get_restriction_qual_cost(root, baserel, param_info, _cost);
startup_cost = qpqual_cost.startup;
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 9cef92cab2..eb0ba6cb1c 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1636,10 +1636,10 @@ set_append_references(PlannerInfo *root,
/*
* See if it's safe to get rid of the Append entirely. For this to be
* safe, there must be only one child plan and that child plan's parallel
- * awareness must match that of the Append's. The
Re: Costing elided SubqueryScans more nearly correctly
On Thu, May 5, 2022 at 4:30 PM Richard Guo wrote: > On Thu, May 5, 2022 at 7:03 AM Tom Lane wrote: >> 1015 improvements to 14 disimprovements isn't a bad score. I'm >> a bit surprised there are that many removable SubqueryScans TBH; >> maybe that's an artifact of all the "SELECT *" queries. > The patch looks sane to me. 1015 vs 14 is a good win. +1 Best regards, Etsuro Fujita
Re: Costing elided SubqueryScans more nearly correctly
On Thu, May 5, 2022 at 7:03 AM Tom Lane wrote: > I wrote: > > I instrumented the code in setrefs.c, and found that during the > > core regression tests this patch estimates correctly in 2103 > > places while guessing wrongly in 54, so that seems like a pretty > > good step forward. > > On second thought, that's not a terribly helpful summary. Breaking > things down to the next level, there were > >1088 places where we correctly guessed a subquery isn't trivial > (so no change from current behavior, which is correct) > >1015 places where we correctly guessed a subquery is trivial > (hence, improving the cost estimate from before) > > 40 places where we incorrectly guessed a subquery isn't trivial > (so no change from current behavior, although that's wrong) > > 14 places where we incorrectly guessed a subquery is trivial > (hence, incorrectly charging zero for the SubqueryScan) > > 1015 improvements to 14 disimprovements isn't a bad score. I'm > a bit surprised there are that many removable SubqueryScans TBH; > maybe that's an artifact of all the "SELECT *" queries. > The patch looks sane to me. 1015 vs 14 is a good win. Thanks Richard
Re: Costing elided SubqueryScans more nearly correctly
I wrote: > I instrumented the code in setrefs.c, and found that during the > core regression tests this patch estimates correctly in 2103 > places while guessing wrongly in 54, so that seems like a pretty > good step forward. On second thought, that's not a terribly helpful summary. Breaking things down to the next level, there were 1088 places where we correctly guessed a subquery isn't trivial (so no change from current behavior, which is correct) 1015 places where we correctly guessed a subquery is trivial (hence, improving the cost estimate from before) 40 places where we incorrectly guessed a subquery isn't trivial (so no change from current behavior, although that's wrong) 14 places where we incorrectly guessed a subquery is trivial (hence, incorrectly charging zero for the SubqueryScan) 1015 improvements to 14 disimprovements isn't a bad score. I'm a bit surprised there are that many removable SubqueryScans TBH; maybe that's an artifact of all the "SELECT *" queries. regards, tom lane
Costing elided SubqueryScans more nearly correctly
In [1] I complained about how SubqueryScans that get deleted from
a plan tree by setrefs.c nonetheless contribute cost increments
that might cause the planner to make odd choices. That turned
out not to be the proximate cause of that particular issue, but
it still seems like it might be a good idea to do something about
it. Here's a little patch to improve matters.
It turns out to be hard to predict perfectly whether setrefs.c will
remove a SubqueryScan, because createplan.c plays some games with
moving tlist evaluations around and/or inserting "physical"
(i.e. trivial) tlists, which can falsify any earlier estimate of
whether a SubqueryScan is trivial. I'm not especially interested in
redesigning those mechanisms, so the best we can hope for is an
approximate determination. (Those same behaviors also make a lot of
other path cost estimates a bit incorrect, so it doesn't seem like
we need to feel too awful about not getting SubqueryScan perfect.)
Given that ground rule, however, it's not very hard to determine
whether a SubqueryScanPath looks like it will be trivial and change
its costing accordingly. The attached draft patch does that.
I instrumented the code in setrefs.c, and found that during the
core regression tests this patch estimates correctly in 2103
places while guessing wrongly in 54, so that seems like a pretty
good step forward.
Perhaps I overcomplicated matters by making the callers responsible
for determining triviality of the paths' targets. We could just
make cost_subqueryscan check that for itself (using logic similar
to what I wrote in set_subquery_pathlist), but that'd result in
duplicative calculations anytime we make more than one Path for a
subquery. On the other hand, said calculations wouldn't be that
expensive, so perhaps a more localized patch would be better.
I also notice that setrefs.c can elide Append and MergeAppend nodes
too in some cases, but AFAICS costing of those node types doesn't
take that into account.
Anyway, I'll stick this in the queue for v16.
regards, tom lane
[1] https://www.postgresql.org/message-id/328872.1651247595%40sss.pgh.pa.us
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
index d84f66a81b..08f4d125e1 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -2439,6 +2439,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
{
Query *parse = root->parse;
Query *subquery = rte->subquery;
+ bool trivial_pathtarget;
Relids required_outer;
pushdown_safety_info safetyInfo;
double tuple_fraction;
@@ -2597,6 +2598,36 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
*/
set_subquery_size_estimates(root, rel);
+ /*
+ * Also detect whether the reltarget is trivial, so that we can pass that
+ * info to cost_subqueryscan (rather than re-deriving it multiple times).
+ * It's trivial if it fetches all the subplan output columns in order.
+ */
+ if (list_length(rel->reltarget->exprs) != list_length(subquery->targetList))
+ trivial_pathtarget = false;
+ else
+ {
+ trivial_pathtarget = true;
+ foreach(lc, rel->reltarget->exprs)
+ {
+ Node *node = (Node *) lfirst(lc);
+ Var *var;
+
+ if (!IsA(node, Var))
+ {
+trivial_pathtarget = false;
+break;
+ }
+ var = (Var *) node;
+ if (var->varno != rti ||
+var->varattno != foreach_current_index(lc) + 1)
+ {
+trivial_pathtarget = false;
+break;
+ }
+ }
+ }
+
/*
* For each Path that subquery_planner produced, make a SubqueryScanPath
* in the outer query.
@@ -2615,6 +2646,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
/* Generate outer path using this subpath */
add_path(rel, (Path *)
create_subqueryscan_path(root, rel, subpath,
+ trivial_pathtarget,
pathkeys, required_outer));
}
@@ -2640,6 +2672,7 @@ set_subquery_pathlist(PlannerInfo *root, RelOptInfo *rel,
/* Generate outer path using this subpath */
add_partial_path(rel, (Path *)
create_subqueryscan_path(root, rel, subpath,
+ trivial_pathtarget,
pathkeys,
required_outer));
}
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 6673d271c2..efb58f083a 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -1388,10 +1388,12 @@ cost_tidrangescan(Path *path, PlannerInfo *root,
*
* 'baserel' is the relation to be scanned
* 'param_info' is the ParamPathInfo if this is a parameterized path, else NULL
+ * 'trivial_pathtarget' is true if the pathtarget is believed to be trivial.
*/
void
cost_subqueryscan(SubqueryScanPath *path, PlannerInfo *root,
- RelOptInfo *baserel, ParamPathInfo *param_info)
+ RelOptInfo *baserel, ParamPathInfo *param_info,
+ bool trivial_pathtarget)
{
Cost startup_cost;
Cost run_cost;
@@ -1431,6
