From: Tom Lane Date: Sun, 26 Jul 2015 20:19:08 +0000 (-0400) Subject: Make entirely-dummy appendrels get marked as such in set_append_rel_size. X-Git-Tag: REL9_5_ALPHA2~63 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=7481c6c2aa379f8d3427819fcaa0eac5c93b1dcf;p=postgresql Make entirely-dummy appendrels get marked as such in set_append_rel_size. The planner generally expects that the estimated rowcount of any relation is at least one row, *unless* it has been proven empty by constraint exclusion or similar mechanisms, which is marked by installing a dummy path as the rel's cheapest path (cf. IS_DUMMY_REL). When I split up allpaths.c's processing of base rels into separate set_base_rel_sizes and set_base_rel_pathlists steps, the intention was that dummy rels would get marked as such during the "set size" step; this is what justifies an Assert in indxpath.c's get_loop_count that other relations should either be dummy or have positive rowcount. Unfortunately I didn't get that quite right for append relations: if all the child rels have been proven empty then set_append_rel_size would come up with a rowcount of zero, which is correct, but it didn't then do set_dummy_rel_pathlist. (We would have ended up with the right state after set_append_rel_pathlist, but that's too late, if we generate indexpaths for some other rel first.) In addition to fixing the actual bug, I installed an Assert enforcing this convention in set_rel_size; that then allows simplification of a couple of now-redundant tests for zero rowcount in set_append_rel_size. Also, to cover the possibility that third-party FDWs have been careless about not returning a zero rowcount estimate, apply clamp_row_est to whatever an FDW comes up with as the rows estimate. Per report from Andreas Seltenreich. Back-patch to 9.2. Earlier branches did not have the separation between set_base_rel_sizes and set_base_rel_pathlists steps, so there was no intermediate state where an appendrel would have had inconsistent rowcount and pathlist. It's possible that adding the Assert to set_rel_size would be a good idea in older branches too; but since they're not under development any more, it's likely not worth the trouble. --- diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 1590be1167..8fc1cfd15f 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -360,6 +360,11 @@ set_rel_size(PlannerInfo *root, RelOptInfo *rel, break; } } + + /* + * We insist that all non-dummy rels have a nonzero rowcount estimate. + */ + Assert(rel->rows > 0 || IS_DUMMY_REL(rel)); } /* @@ -579,6 +584,9 @@ set_foreign_size(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte) /* Let FDW adjust the size estimates, if it can */ rel->fdwroutine->GetForeignRelSize(root, rel, rte->relid); + + /* ... but do not let it set the rows estimate to zero */ + rel->rows = clamp_row_est(rel->rows); } /* @@ -608,6 +616,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, Index rti, RangeTblEntry *rte) { int parentRTindex = rti; + bool has_live_children; double parent_rows; double parent_size; double *parent_attrsizes; @@ -628,6 +637,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, * Note: if you consider changing this logic, beware that child rels could * have zero rows and/or width, if they were excluded by constraints. */ + has_live_children = false; parent_rows = 0; parent_size = 0; nattrs = rel->max_attr - rel->min_attr + 1; @@ -755,70 +765,80 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel, if (IS_DUMMY_REL(childrel)) continue; + /* We have at least one live child. */ + has_live_children = true; + /* * Accumulate size information from each live child. */ - if (childrel->rows > 0) + Assert(childrel->rows > 0); + + parent_rows += childrel->rows; + parent_size += childrel->width * childrel->rows; + + /* + * Accumulate per-column estimates too. We need not do anything for + * PlaceHolderVars in the parent list. If child expression isn't a + * Var, or we didn't record a width estimate for it, we have to fall + * back on a datatype-based estimate. + * + * By construction, child's reltargetlist is 1-to-1 with parent's. + */ + forboth(parentvars, rel->reltargetlist, + childvars, childrel->reltargetlist) { - parent_rows += childrel->rows; - parent_size += childrel->width * childrel->rows; + Var *parentvar = (Var *) lfirst(parentvars); + Node *childvar = (Node *) lfirst(childvars); - /* - * Accumulate per-column estimates too. We need not do anything - * for PlaceHolderVars in the parent list. If child expression - * isn't a Var, or we didn't record a width estimate for it, we - * have to fall back on a datatype-based estimate. - * - * By construction, child's reltargetlist is 1-to-1 with parent's. - */ - forboth(parentvars, rel->reltargetlist, - childvars, childrel->reltargetlist) + if (IsA(parentvar, Var)) { - Var *parentvar = (Var *) lfirst(parentvars); - Node *childvar = (Node *) lfirst(childvars); + int pndx = parentvar->varattno - rel->min_attr; + int32 child_width = 0; - if (IsA(parentvar, Var)) + if (IsA(childvar, Var) && + ((Var *) childvar)->varno == childrel->relid) { - int pndx = parentvar->varattno - rel->min_attr; - int32 child_width = 0; + int cndx = ((Var *) childvar)->varattno - childrel->min_attr; - if (IsA(childvar, Var) && - ((Var *) childvar)->varno == childrel->relid) - { - int cndx = ((Var *) childvar)->varattno - childrel->min_attr; - - child_width = childrel->attr_widths[cndx]; - } - if (child_width <= 0) - child_width = get_typavgwidth(exprType(childvar), - exprTypmod(childvar)); - Assert(child_width > 0); - parent_attrsizes[pndx] += child_width * childrel->rows; + child_width = childrel->attr_widths[cndx]; } + if (child_width <= 0) + child_width = get_typavgwidth(exprType(childvar), + exprTypmod(childvar)); + Assert(child_width > 0); + parent_attrsizes[pndx] += child_width * childrel->rows; } } } - /* - * Save the finished size estimates. - */ - rel->rows = parent_rows; - if (parent_rows > 0) + if (has_live_children) { + /* + * Save the finished size estimates. + */ int i; + Assert(parent_rows > 0); + rel->rows = parent_rows; rel->width = rint(parent_size / parent_rows); for (i = 0; i < nattrs; i++) rel->attr_widths[i] = rint(parent_attrsizes[i] / parent_rows); + + /* + * Set "raw tuples" count equal to "rows" for the appendrel; needed + * because some places assume rel->tuples is valid for any baserel. + */ + rel->tuples = parent_rows; } else - rel->width = 0; /* attr_widths should be zero already */ - - /* - * Set "raw tuples" count equal to "rows" for the appendrel; needed - * because some places assume rel->tuples is valid for any baserel. - */ - rel->tuples = parent_rows; + { + /* + * All children were excluded by constraints, so mark the whole + * appendrel dummy. We must do this in this phase so that the rel's + * dummy-ness is visible when we generate paths for other rels. + */ + set_dummy_rel_pathlist(rel); + } pfree(parent_attrsizes); } diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 139f7e0498..96ec997ed1 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2164,6 +2164,26 @@ select count(*) from tenk1 x where (1 row) rollback; +-- +-- regression test: be sure we cope with proven-dummy append rels +-- +explain (costs off) +select aa, bb, unique1, unique1 + from tenk1 right join b on aa = unique1 + where bb < bb and bb is null; + QUERY PLAN +-------------------------- + Result + One-Time Filter: false +(2 rows) + +select aa, bb, unique1, unique1 + from tenk1 right join b on aa = unique1 + where bb < bb and bb is null; + aa | bb | unique1 | unique1 +----+----+---------+--------- +(0 rows) + -- -- Clean up -- diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 5b65ea8c92..ada78db264 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -353,6 +353,17 @@ select count(*) from tenk1 x where x.unique1 in (select aa.f1 from int4_tbl aa,float8_tbl bb where aa.f1=bb.f1); rollback; +-- +-- regression test: be sure we cope with proven-dummy append rels +-- +explain (costs off) +select aa, bb, unique1, unique1 + from tenk1 right join b on aa = unique1 + where bb < bb and bb is null; + +select aa, bb, unique1, unique1 + from tenk1 right join b on aa = unique1 + where bb < bb and bb is null; -- -- Clean up @@ -1120,6 +1131,7 @@ select atts.relid::regclass, s.* from pg_stats s join a.attrelid::regclass::text join (select unnest(indkey) attnum, indexrelid from pg_index i) atts on atts.attnum = a.attnum where schemaname != 'pg_catalog'; + -- -- Test LATERAL --