Skip to content

Commit 19a5411

Browse files
tglsfdctglsfdc
committed
Add an explicit representation of the output targetlist to Paths.
Up to now, there's been an assumption that all Paths for a given relation compute the same output column set (targetlist). However, there are good reasons to remove that assumption. For example, an indexscan on an expression index might be able to return the value of an expensive function "for free". While we have the ability to generate such a plan today in simple cases, we don't have a way to model that it's cheaper than a plan that computes the function from scratch, nor a way to create such a plan in join cases (where the function computation would normally happen at the topmost join node). Also, we need this so that we can have Paths representing post-scan/join steps, where the targetlist may well change from one step to the next. Therefore, invent a "struct PathTarget" representing the columns we expect a plan step to emit. It's convenient to include the output tuple width and tlist evaluation cost in this struct, and there will likely be additional fields in future. While Path nodes that actually do have custom outputs will need their own PathTargets, it will still be true that most Paths for a given relation will compute the same tlist. To reduce the overhead added by this patch, keep a "default PathTarget" in RelOptInfo, and allow Paths that compute that column set to just point to their parent RelOptInfo's reltarget. (In the patch as committed, actually every Path is like that, since we do not yet have any cases of custom PathTargets.) I took this opportunity to provide some more-honest costing of PlaceHolderVar evaluation. Up to now, the assumption that "scan/join reltargetlists have cost zero" was applied not only to Vars, where it's reasonable, but also PlaceHolderVars where it isn't. Now, we add the eval cost of a PlaceHolderVar's expression to the first plan level where it can be computed, by including it in the PathTarget cost field and adding that to the cost estimates for Paths. This isn't perfect yet but it's much better than before, and there is a way forward to improve it more. This costing change affects the join order chosen for a couple of the regression tests, changing expected row ordering.
1 parent 3386f34 commit 19a5411

File tree

18 files changed

+326
-143
lines changed

18 files changed

+326
-143
lines changed

‎contrib/file_fdw/file_fdw.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -806,7 +806,7 @@ check_selective_binary_conversion(RelOptInfo *baserel,
806806
}
807807

808808
/* Collect all the attributes needed for joins or final output. */
809-
pull_varattnos((Node *) baserel->reltargetlist, baserel->relid,
809+
pull_varattnos((Node *) baserel->reltarget.exprs, baserel->relid,
810810
&attrs_used);
811811

812812
/* Add all the attributes used by restriction clauses. */
@@ -938,7 +938,7 @@ estimate_size(PlannerInfo *root, RelOptInfo *baserel,
938938
*/
939939
int tuple_width;
940940

941-
tuple_width = MAXALIGN(baserel->width) +
941+
tuple_width = MAXALIGN(baserel->reltarget.width) +
942942
MAXALIGN(SizeofHeapTupleHeader);
943943
ntuples = clamp_row_est((double) stat_buf.st_size /
944944
(double) tuple_width);

‎contrib/postgres_fdw/deparse.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -728,10 +728,10 @@ build_tlist_to_deparse(RelOptInfo *foreignrel)
728728
PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
729729

730730
/*
731-
* We require columns specified in foreignrel->reltargetlist and those
731+
* We require columns specified in foreignrel->reltarget.exprs and those
732732
* required for evaluating the local conditions.
733733
*/
734-
tlist = add_to_flat_tlist(tlist, foreignrel->reltargetlist);
734+
tlist = add_to_flat_tlist(tlist, foreignrel->reltarget.exprs);
735735
tlist = add_to_flat_tlist(tlist,
736736
pull_var_clause((Node *) fpinfo->local_conds,
737737
PVC_REJECT_AGGREGATES,

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -479,7 +479,7 @@ postgresGetForeignRelSize(PlannerInfo *root,
479479
* columns used in them. Doesn't seem worth detecting that case though.)
480480
*/
481481
fpinfo->attrs_used = NULL;
482-
pull_varattnos((Node *) baserel->reltargetlist, baserel->relid,
482+
pull_varattnos((Node *) baserel->reltarget.exprs, baserel->relid,
483483
&fpinfo->attrs_used);
484484
foreach(lc, fpinfo->local_conds)
485485
{
@@ -522,7 +522,7 @@ postgresGetForeignRelSize(PlannerInfo *root,
522522

523523
/* Report estimated baserel size to planner. */
524524
baserel->rows = fpinfo->rows;
525-
baserel->width = fpinfo->width;
525+
baserel->reltarget.width = fpinfo->width;
526526
}
527527
else
528528
{
@@ -539,7 +539,8 @@ postgresGetForeignRelSize(PlannerInfo *root,
539539
{
540540
baserel->pages = 10;
541541
baserel->tuples =
542-
(10 * BLCKSZ) / (baserel->width + MAXALIGN(SizeofHeapTupleHeader));
542+
(10 * BLCKSZ) / (baserel->reltarget.width +
543+
MAXALIGN(SizeofHeapTupleHeader));
543544
}
544545

545546
/* Estimate baserel size as best we can with local statistics. */
@@ -2176,7 +2177,7 @@ estimate_path_cost_size(PlannerInfo *root,
21762177
* between foreign relations.
21772178
*/
21782179
rows = foreignrel->rows;
2179-
width = foreignrel->width;
2180+
width = foreignrel->reltarget.width;
21802181

21812182
/* Back into an estimate of the number of retrieved rows. */
21822183
retrieved_rows = clamp_row_est(rows / fpinfo->local_conds_sel);
@@ -3646,7 +3647,7 @@ postgresGetForeignJoinPaths(PlannerInfo *root,
36463647
&width, &startup_cost, &total_cost);
36473648
/* Now update this information in the joinrel */
36483649
joinrel->rows = rows;
3649-
joinrel->width = width;
3650+
joinrel->reltarget.width = width;
36503651
fpinfo->rows = rows;
36513652
fpinfo->width = width;
36523653
fpinfo->startup_cost = startup_cost;

‎doc/src/sgml/fdwhandler.sgml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1159,7 +1159,7 @@ GetForeignServerByName(const char *name, bool missing_ok);
11591159
it contains restriction quals (<literal>WHERE</> clauses) that should be
11601160
used to filter the rows to be fetched. (The FDW itself is not required
11611161
to enforce these quals, as the core executor can check them instead.)
1162-
<literal>baserel-&gt;reltargetlist</> can be used to determine which
1162+
<literal>baserel-&gt;reltarget.exprs</> can be used to determine which
11631163
columns need to be fetched; but note that it only lists columns that
11641164
have to be emitted by the <structname>ForeignScan</> plan node, not
11651165
columns that are used in qual evaluation but not output by the query.

‎src/backend/nodes/outfuncs.c

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1589,6 +1589,7 @@ _outOnConflictExpr(StringInfo str, const OnConflictExpr *node)
15891589
*
15901590
* Note we do NOT print the parent, else we'd be in infinite recursion.
15911591
* We can print the parent's relids for identification purposes, though.
1592+
* We print the pathtarget only if it's not the default one for the rel.
15921593
* We also do not print the whole of param_info, since it's printed by
15931594
* _outRelOptInfo; it's sufficient and less cluttering to print just the
15941595
* required outer relids.
@@ -1598,10 +1599,14 @@ _outPathInfo(StringInfo str, const Path *node)
15981599
{
15991600
WRITE_ENUM_FIELD(pathtype, NodeTag);
16001601
appendStringInfoString(str, " :parent_relids ");
1601-
if (node->parent)
1602-
_outBitmapset(str, node->parent->relids);
1603-
else
1604-
_outBitmapset(str, NULL);
1602+
_outBitmapset(str, node->parent->relids);
1603+
if (node->pathtarget != &(node->parent->reltarget))
1604+
{
1605+
WRITE_NODE_FIELD(pathtarget->exprs);
1606+
WRITE_FLOAT_FIELD(pathtarget->cost.startup, "%.2f");
1607+
WRITE_FLOAT_FIELD(pathtarget->cost.per_tuple, "%.2f");
1608+
WRITE_INT_FIELD(pathtarget->width);
1609+
}
16051610
appendStringInfoString(str, " :required_outer ");
16061611
if (node->param_info)
16071612
_outBitmapset(str, node->param_info->ppi_req_outer);
@@ -1901,11 +1906,13 @@ _outRelOptInfo(StringInfo str, const RelOptInfo *node)
19011906
WRITE_ENUM_FIELD(reloptkind, RelOptKind);
19021907
WRITE_BITMAPSET_FIELD(relids);
19031908
WRITE_FLOAT_FIELD(rows, "%.0f");
1904-
WRITE_INT_FIELD(width);
19051909
WRITE_BOOL_FIELD(consider_startup);
19061910
WRITE_BOOL_FIELD(consider_param_startup);
19071911
WRITE_BOOL_FIELD(consider_parallel);
1908-
WRITE_NODE_FIELD(reltargetlist);
1912+
WRITE_NODE_FIELD(reltarget.exprs);
1913+
WRITE_FLOAT_FIELD(reltarget.cost.startup, "%.2f");
1914+
WRITE_FLOAT_FIELD(reltarget.cost.per_tuple, "%.2f");
1915+
WRITE_INT_FIELD(reltarget.width);
19091916
WRITE_NODE_FIELD(pathlist);
19101917
WRITE_NODE_FIELD(ppilist);
19111918
WRITE_NODE_FIELD(partial_pathlist);

‎src/backend/optimizer/path/allpaths.c

Lines changed: 19 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -919,19 +919,20 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
919919
/*
920920
* CE failed, so finish copying/modifying targetlist and join quals.
921921
*
922-
* Note: the resulting childrel->reltargetlist may contain arbitrary
923-
* expressions, which otherwise would not occur in a reltargetlist.
922+
* Note: the resulting childrel->reltarget.exprs may contain arbitrary
923+
* expressions, which otherwise would not occur in a rel's targetlist.
924924
* Code that might be looking at an appendrel child must cope with
925-
* such. (Normally, a reltargetlist would only include Vars and
926-
* PlaceHolderVars.)
925+
* such. (Normally, a rel's targetlist would only include Vars and
926+
* PlaceHolderVars.) XXX we do not bother to update the cost or width
927+
* fields of childrel->reltarget; not clear if that would be useful.
927928
*/
928929
childrel->joininfo = (List *)
929930
adjust_appendrel_attrs(root,
930931
(Node *) rel->joininfo,
931932
appinfo);
932-
childrel->reltargetlist = (List *)
933+
childrel->reltarget.exprs = (List *)
933934
adjust_appendrel_attrs(root,
934-
(Node *) rel->reltargetlist,
935+
(Node *) rel->reltarget.exprs,
935936
appinfo);
936937

937938
/*
@@ -976,18 +977,18 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
976977
Assert(childrel->rows > 0);
977978

978979
parent_rows += childrel->rows;
979-
parent_size += childrel->width * childrel->rows;
980+
parent_size += childrel->reltarget.width * childrel->rows;
980981

981982
/*
982983
* Accumulate per-column estimates too. We need not do anything for
983984
* PlaceHolderVars in the parent list. If child expression isn't a
984985
* Var, or we didn't record a width estimate for it, we have to fall
985986
* back on a datatype-based estimate.
986987
*
987-
* By construction, child's reltargetlist is 1-to-1 with parent's.
988+
* By construction, child's targetlist is 1-to-1 with parent's.
988989
*/
989-
forboth(parentvars, rel->reltargetlist,
990-
childvars, childrel->reltargetlist)
990+
forboth(parentvars, rel->reltarget.exprs,
991+
childvars, childrel->reltarget.exprs)
991992
{
992993
Var *parentvar = (Var *) lfirst(parentvars);
993994
Node *childvar = (Node *) lfirst(childvars);
@@ -1022,7 +1023,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
10221023

10231024
Assert(parent_rows > 0);
10241025
rel->rows = parent_rows;
1025-
rel->width = rint(parent_size / parent_rows);
1026+
rel->reltarget.width = rint(parent_size / parent_rows);
10261027
for (i = 0; i < nattrs; i++)
10271028
rel->attr_widths[i] = rint(parent_attrsizes[i] / parent_rows);
10281029

@@ -1495,7 +1496,7 @@ set_dummy_rel_pathlist(RelOptInfo *rel)
14951496
{
14961497
/* Set dummy size estimates --- we leave attr_widths[] as zeroes */
14971498
rel->rows = 0;
1498-
rel->width = 0;
1499+
rel->reltarget.width = 0;
14991500

15001501
/* Discard any pre-existing paths; no further need for them */
15011502
rel->pathlist = NIL;
@@ -1728,11 +1729,11 @@ set_function_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
17281729
ListCell *lc;
17291730

17301731
/*
1731-
* Is there a Var for it in reltargetlist? If not, the query did not
1732-
* reference the ordinality column, or at least not in any way that
1733-
* would be interesting for sorting.
1732+
* Is there a Var for it in rel's targetlist? If not, the query did
1733+
* not reference the ordinality column, or at least not in any way
1734+
* that would be interesting for sorting.
17341735
*/
1735-
foreach(lc, rel->reltargetlist)
1736+
foreach(lc, rel->reltarget.exprs)
17361737
{
17371738
Var *node = (Var *) lfirst(lc);
17381739

@@ -2676,11 +2677,11 @@ remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
26762677
* query.
26772678
*
26782679
* Add all the attributes needed for joins or final output. Note: we must
2679-
* look at reltargetlist, not the attr_needed data, because attr_needed
2680+
* look at rel's targetlist, not the attr_needed data, because attr_needed
26802681
* isn't computed for inheritance child rels, cf set_append_rel_size().
26812682
* (XXX might be worth changing that sometime.)
26822683
*/
2683-
pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
2684+
pull_varattnos((Node *) rel->reltarget.exprs, rel->relid, &attrs_used);
26842685

26852686
/* Add all the attributes used by un-pushed-down restriction clauses. */
26862687
foreach(lc, rel->baserestrictinfo)

0 commit comments

Comments
 (0)