Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 8352b14

Browse files
committed
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.
1 parent 84bf6ec commit 8352b14

File tree

3 files changed

+93
-42
lines changed

3 files changed

+93
-42
lines changed

src/backend/optimizer/path/allpaths.c

Lines changed: 62 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,11 @@ set_rel_size(PlannerInfo *root, RelOptInfo *rel,
333333
break;
334334
}
335335
}
336+
337+
/*
338+
* We insist that all non-dummy rels have a nonzero rowcount estimate.
339+
*/
340+
Assert(rel->rows > 0 || IS_DUMMY_REL(rel));
336341
}
337342

338343
/*
@@ -462,6 +467,9 @@ set_foreign_size(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte)
462467

463468
/* Let FDW adjust the size estimates, if it can */
464469
rel->fdwroutine->GetForeignRelSize(root, rel, rte->relid);
470+
471+
/* ... but do not let it set the rows estimate to zero */
472+
rel->rows = clamp_row_est(rel->rows);
465473
}
466474

467475
/*
@@ -494,6 +502,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
494502
Index rti, RangeTblEntry *rte)
495503
{
496504
int parentRTindex = rti;
505+
bool has_live_children;
497506
double parent_rows;
498507
double parent_size;
499508
double *parent_attrsizes;
@@ -514,6 +523,7 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
514523
* Note: if you consider changing this logic, beware that child rels could
515524
* have zero rows and/or width, if they were excluded by constraints.
516525
*/
526+
has_live_children = false;
517527
parent_rows = 0;
518528
parent_size = 0;
519529
nattrs = rel->max_attr - rel->min_attr + 1;
@@ -641,70 +651,80 @@ set_append_rel_size(PlannerInfo *root, RelOptInfo *rel,
641651
if (IS_DUMMY_REL(childrel))
642652
continue;
643653

654+
/* We have at least one live child. */
655+
has_live_children = true;
656+
644657
/*
645658
* Accumulate size information from each live child.
646659
*/
647-
if (childrel->rows > 0)
660+
Assert(childrel->rows > 0);
661+
662+
parent_rows += childrel->rows;
663+
parent_size += childrel->width * childrel->rows;
664+
665+
/*
666+
* Accumulate per-column estimates too. We need not do anything for
667+
* PlaceHolderVars in the parent list. If child expression isn't a
668+
* Var, or we didn't record a width estimate for it, we have to fall
669+
* back on a datatype-based estimate.
670+
*
671+
* By construction, child's reltargetlist is 1-to-1 with parent's.
672+
*/
673+
forboth(parentvars, rel->reltargetlist,
674+
childvars, childrel->reltargetlist)
648675
{
649-
parent_rows += childrel->rows;
650-
parent_size += childrel->width * childrel->rows;
676+
Var *parentvar = (Var *) lfirst(parentvars);
677+
Node *childvar = (Node *) lfirst(childvars);
651678

652-
/*
653-
* Accumulate per-column estimates too. We need not do anything
654-
* for PlaceHolderVars in the parent list. If child expression
655-
* isn't a Var, or we didn't record a width estimate for it, we
656-
* have to fall back on a datatype-based estimate.
657-
*
658-
* By construction, child's reltargetlist is 1-to-1 with parent's.
659-
*/
660-
forboth(parentvars, rel->reltargetlist,
661-
childvars, childrel->reltargetlist)
679+
if (IsA(parentvar, Var))
662680
{
663-
Var *parentvar = (Var *) lfirst(parentvars);
664-
Node *childvar = (Node *) lfirst(childvars);
681+
int pndx = parentvar->varattno - rel->min_attr;
682+
int32 child_width = 0;
665683

666-
if (IsA(parentvar, Var))
684+
if (IsA(childvar, Var) &&
685+
((Var *) childvar)->varno == childrel->relid)
667686
{
668-
int pndx = parentvar->varattno - rel->min_attr;
669-
int32 child_width = 0;
687+
int cndx = ((Var *) childvar)->varattno - childrel->min_attr;
670688

671-
if (IsA(childvar, Var) &&
672-
((Var *) childvar)->varno == childrel->relid)
673-
{
674-
int cndx = ((Var *) childvar)->varattno - childrel->min_attr;
675-
676-
child_width = childrel->attr_widths[cndx];
677-
}
678-
if (child_width <= 0)
679-
child_width = get_typavgwidth(exprType(childvar),
680-
exprTypmod(childvar));
681-
Assert(child_width > 0);
682-
parent_attrsizes[pndx] += child_width * childrel->rows;
689+
child_width = childrel->attr_widths[cndx];
683690
}
691+
if (child_width <= 0)
692+
child_width = get_typavgwidth(exprType(childvar),
693+
exprTypmod(childvar));
694+
Assert(child_width > 0);
695+
parent_attrsizes[pndx] += child_width * childrel->rows;
684696
}
685697
}
686698
}
687699

688-
/*
689-
* Save the finished size estimates.
690-
*/
691-
rel->rows = parent_rows;
692-
if (parent_rows > 0)
700+
if (has_live_children)
693701
{
702+
/*
703+
* Save the finished size estimates.
704+
*/
694705
int i;
695706

707+
Assert(parent_rows > 0);
708+
rel->rows = parent_rows;
696709
rel->width = rint(parent_size / parent_rows);
697710
for (i = 0; i < nattrs; i++)
698711
rel->attr_widths[i] = rint(parent_attrsizes[i] / parent_rows);
712+
713+
/*
714+
* Set "raw tuples" count equal to "rows" for the appendrel; needed
715+
* because some places assume rel->tuples is valid for any baserel.
716+
*/
717+
rel->tuples = parent_rows;
699718
}
700719
else
701-
rel->width = 0; /* attr_widths should be zero already */
702-
703-
/*
704-
* Set "raw tuples" count equal to "rows" for the appendrel; needed
705-
* because some places assume rel->tuples is valid for any baserel.
706-
*/
707-
rel->tuples = parent_rows;
720+
{
721+
/*
722+
* All children were excluded by constraints, so mark the whole
723+
* appendrel dummy. We must do this in this phase so that the rel's
724+
* dummy-ness is visible when we generate paths for other rels.
725+
*/
726+
set_dummy_rel_pathlist(rel);
727+
}
708728

709729
pfree(parent_attrsizes);
710730
}

src/test/regress/expected/join.out

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2164,6 +2164,26 @@ select count(*) from tenk1 x where
21642164
(1 row)
21652165

21662166
rollback;
2167+
--
2168+
-- regression test: be sure we cope with proven-dummy append rels
2169+
--
2170+
explain (costs off)
2171+
select aa, bb, unique1, unique1
2172+
from tenk1 right join b on aa = unique1
2173+
where bb < bb and bb is null;
2174+
QUERY PLAN
2175+
--------------------------
2176+
Result
2177+
One-Time Filter: false
2178+
(2 rows)
2179+
2180+
select aa, bb, unique1, unique1
2181+
from tenk1 right join b on aa = unique1
2182+
where bb < bb and bb is null;
2183+
aa | bb | unique1 | unique1
2184+
----+----+---------+---------
2185+
(0 rows)
2186+
21672187
--
21682188
-- Clean up
21692189
--

src/test/regress/sql/join.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,17 @@ select count(*) from tenk1 x where
353353
x.unique1 in (select aa.f1 from int4_tbl aa,float8_tbl bb where aa.f1=bb.f1);
354354
rollback;
355355

356+
--
357+
-- regression test: be sure we cope with proven-dummy append rels
358+
--
359+
explain (costs off)
360+
select aa, bb, unique1, unique1
361+
from tenk1 right join b on aa = unique1
362+
where bb < bb and bb is null;
363+
364+
select aa, bb, unique1, unique1
365+
from tenk1 right join b on aa = unique1
366+
where bb < bb and bb is null;
356367

357368
--
358369
-- Clean up

0 commit comments

Comments
 (0)