Skip to content

Commit de797e8

Browse files
tglsfdctglsfdc
committed
Fix bitmap AND/OR scans on the inside of a nestloop partition-wise join.
reparameterize_path_by_child() failed to reparameterize BitmapAnd and BitmapOr paths. This matters only if such a path is chosen as the inside of a nestloop partition-wise join, where we have to pass in parameters from the outside of the nestloop. If that did happen, we generated a bad plan that would likely lead to crashes at execution. This is not entirely reparameterize_path_by_child()'s fault though; it's the victim of an ancient decision (my ancient decision, I think) to not bother filling in param_info in BitmapAnd/Or path nodes. That caused the function to believe that such nodes and their children contain no parameter references and so need not be processed. In hindsight that decision looks pretty penny-wise and pound-foolish: while it saves a few cycles during path node setup, we do commonly need the information later. In particular, by reversing the decision and requiring valid param_info data in all nodes of a bitmap path tree, we can get rid of indxpath.c's get_bitmap_tree_required_outer() function, which computed the data on-demand. It's not unlikely that that nets out as a savings of cycles in many scenarios. A couple of other things in indxpath.c can be simplified as well. While here, get rid of some cases in reparameterize_path_by_child() that are visibly dead or useless, given that we only care about reparameterizing paths that can be on the inside of a parameterized nestloop. This case reminds one of the maxim that untested code probably does not work, so I'm unwilling to leave unreachable code in this function. (I did leave the T_Gather case in place even though it's not reached in the regression tests. It's not very clear to me when the planner might prefer to put Gather below rather than above a nestloop, but at least in principle the case might be interesting.) Per bug #16536, originally from Arne Roland but with a test case by Andrew Gierth. Back-patch to v11 where this code came in. Discussion: https://postgr.es/m/16536-2213ee0b3aad41fd@postgresql.org
1 parent 1231a0b commit de797e8

File tree

4 files changed

+212
-158
lines changed

4 files changed

+212
-158
lines changed

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

Lines changed: 17 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,6 @@ static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel,
122122
List *paths);
123123
static PathClauseUsage *classify_index_clause_usage(Path *path,
124124
List **clauselist);
125-
static Relids get_bitmap_tree_required_outer(Path *bitmapqual);
126125
static void find_indexpath_quals(Path *bitmapqual, List **quals, List **preds);
127126
static int find_list_position(Node *node, List **nodelist);
128127
static bool check_index_only(RelOptInfo *rel, IndexOptInfo *index);
@@ -357,23 +356,16 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
357356
*/
358357
if (bitjoinpaths != NIL)
359358
{
360-
List *path_outer;
361359
List *all_path_outers;
362360
ListCell *lc;
363361

364-
/*
365-
* path_outer holds the parameterization of each path in bitjoinpaths
366-
* (to save recalculating that several times), while all_path_outers
367-
* holds all distinct parameterization sets.
368-
*/
369-
path_outer = all_path_outers = NIL;
362+
/* Identify each distinct parameterization seen in bitjoinpaths */
363+
all_path_outers = NIL;
370364
foreach(lc, bitjoinpaths)
371365
{
372366
Path *path = (Path *) lfirst(lc);
373-
Relids required_outer;
367+
Relids required_outer = PATH_REQ_OUTER(path);
374368

375-
required_outer = get_bitmap_tree_required_outer(path);
376-
path_outer = lappend(path_outer, required_outer);
377369
if (!bms_equal_any(required_outer, all_path_outers))
378370
all_path_outers = lappend(all_path_outers, required_outer);
379371
}
@@ -388,16 +380,14 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
388380
double loop_count;
389381
BitmapHeapPath *bpath;
390382
ListCell *lcp;
391-
ListCell *lco;
392383

393384
/* Identify all the bitmap join paths needing no more than that */
394385
this_path_set = NIL;
395-
forboth(lcp, bitjoinpaths, lco, path_outer)
386+
foreach(lcp, bitjoinpaths)
396387
{
397388
Path *path = (Path *) lfirst(lcp);
398-
Relids p_outers = (Relids) lfirst(lco);
399389

400-
if (bms_is_subset(p_outers, max_outers))
390+
if (bms_is_subset(PATH_REQ_OUTER(path), max_outers))
401391
this_path_set = lappend(this_path_set, path);
402392
}
403393

@@ -411,7 +401,7 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
411401
bitmapqual = choose_bitmap_and(root, rel, this_path_set);
412402

413403
/* And push that path into the mix */
414-
required_outer = get_bitmap_tree_required_outer(bitmapqual);
404+
required_outer = PATH_REQ_OUTER(bitmapqual);
415405
loop_count = get_loop_count(root, rel->relid, required_outer);
416406
bpath = create_bitmap_heap_path(root, rel, bitmapqual,
417407
required_outer, loop_count, 0);
@@ -1609,25 +1599,19 @@ path_usage_comparator(const void *a, const void *b)
16091599

16101600
/*
16111601
* Estimate the cost of actually executing a bitmap scan with a single
1612-
* index path (no BitmapAnd, at least not at this level; but it could be
1613-
* a BitmapOr).
1602+
* index path (which could be a BitmapAnd or BitmapOr node).
16141603
*/
16151604
static Cost
16161605
bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
16171606
{
16181607
BitmapHeapPath bpath;
1619-
Relids required_outer;
1620-
1621-
/* Identify required outer rels, in case it's a parameterized scan */
1622-
required_outer = get_bitmap_tree_required_outer(ipath);
16231608

16241609
/* Set up a dummy BitmapHeapPath */
16251610
bpath.path.type = T_BitmapHeapPath;
16261611
bpath.path.pathtype = T_BitmapHeapScan;
16271612
bpath.path.parent = rel;
16281613
bpath.path.pathtarget = rel->reltarget;
1629-
bpath.path.param_info = get_baserel_parampathinfo(root, rel,
1630-
required_outer);
1614+
bpath.path.param_info = ipath->param_info;
16311615
bpath.path.pathkeys = NIL;
16321616
bpath.bitmapqual = ipath;
16331617

@@ -1636,10 +1620,13 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
16361620
* Parallel bitmap heap path will be considered at later stage.
16371621
*/
16381622
bpath.path.parallel_workers = 0;
1623+
1624+
/* Now we can do cost_bitmap_heap_scan */
16391625
cost_bitmap_heap_scan(&bpath.path, root, rel,
16401626
bpath.path.param_info,
16411627
ipath,
1642-
get_loop_count(root, rel->relid, required_outer));
1628+
get_loop_count(root, rel->relid,
1629+
PATH_REQ_OUTER(ipath)));
16431630

16441631
return bpath.path.total_cost;
16451632
}
@@ -1651,46 +1638,15 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
16511638
static Cost
16521639
bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel, List *paths)
16531640
{
1654-
BitmapAndPath apath;
1655-
BitmapHeapPath bpath;
1656-
Relids required_outer;
1657-
1658-
/* Set up a dummy BitmapAndPath */
1659-
apath.path.type = T_BitmapAndPath;
1660-
apath.path.pathtype = T_BitmapAnd;
1661-
apath.path.parent = rel;
1662-
apath.path.pathtarget = rel->reltarget;
1663-
apath.path.param_info = NULL; /* not used in bitmap trees */
1664-
apath.path.pathkeys = NIL;
1665-
apath.bitmapquals = paths;
1666-
cost_bitmap_and_node(&apath, root);
1667-
1668-
/* Identify required outer rels, in case it's a parameterized scan */
1669-
required_outer = get_bitmap_tree_required_outer((Path *) &apath);
1670-
1671-
/* Set up a dummy BitmapHeapPath */
1672-
bpath.path.type = T_BitmapHeapPath;
1673-
bpath.path.pathtype = T_BitmapHeapScan;
1674-
bpath.path.parent = rel;
1675-
bpath.path.pathtarget = rel->reltarget;
1676-
bpath.path.param_info = get_baserel_parampathinfo(root, rel,
1677-
required_outer);
1678-
bpath.path.pathkeys = NIL;
1679-
bpath.bitmapqual = (Path *) &apath;
1641+
BitmapAndPath *apath;
16801642

16811643
/*
1682-
* Check the cost of temporary path without considering parallelism.
1683-
* Parallel bitmap heap path will be considered at later stage.
1644+
* Might as well build a real BitmapAndPath here, as the work is slightly
1645+
* too complicated to be worth repeating just to save one palloc.
16841646
*/
1685-
bpath.path.parallel_workers = 0;
1686-
1687-
/* Now we can do cost_bitmap_heap_scan */
1688-
cost_bitmap_heap_scan(&bpath.path, root, rel,
1689-
bpath.path.param_info,
1690-
(Path *) &apath,
1691-
get_loop_count(root, rel->relid, required_outer));
1647+
apath = create_bitmap_and_path(root, rel, paths);
16921648

1693-
return bpath.path.total_cost;
1649+
return bitmap_scan_cost_est(root, rel, (Path *) apath);
16941650
}
16951651

16961652

@@ -1761,49 +1717,6 @@ classify_index_clause_usage(Path *path, List **clauselist)
17611717
}
17621718

17631719

1764-
/*
1765-
* get_bitmap_tree_required_outer
1766-
* Find the required outer rels for a bitmap tree (index/and/or)
1767-
*
1768-
* We don't associate any particular parameterization with a BitmapAnd or
1769-
* BitmapOr node; however, the IndexPaths have parameterization info, in
1770-
* their capacity as standalone access paths. The parameterization required
1771-
* for the bitmap heap scan node is the union of rels referenced in the
1772-
* child IndexPaths.
1773-
*/
1774-
static Relids
1775-
get_bitmap_tree_required_outer(Path *bitmapqual)
1776-
{
1777-
Relids result = NULL;
1778-
ListCell *lc;
1779-
1780-
if (IsA(bitmapqual, IndexPath))
1781-
{
1782-
return bms_copy(PATH_REQ_OUTER(bitmapqual));
1783-
}
1784-
else if (IsA(bitmapqual, BitmapAndPath))
1785-
{
1786-
foreach(lc, ((BitmapAndPath *) bitmapqual)->bitmapquals)
1787-
{
1788-
result = bms_join(result,
1789-
get_bitmap_tree_required_outer((Path *) lfirst(lc)));
1790-
}
1791-
}
1792-
else if (IsA(bitmapqual, BitmapOrPath))
1793-
{
1794-
foreach(lc, ((BitmapOrPath *) bitmapqual)->bitmapquals)
1795-
{
1796-
result = bms_join(result,
1797-
get_bitmap_tree_required_outer((Path *) lfirst(lc)));
1798-
}
1799-
}
1800-
else
1801-
elog(ERROR, "unrecognized node type: %d", nodeTag(bitmapqual));
1802-
1803-
return result;
1804-
}
1805-
1806-
18071720
/*
18081721
* find_indexpath_quals
18091722
*

‎src/backend/optimizer/util/pathnode.c

Lines changed: 46 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1107,11 +1107,27 @@ create_bitmap_and_path(PlannerInfo *root,
11071107
List *bitmapquals)
11081108
{
11091109
BitmapAndPath *pathnode = makeNode(BitmapAndPath);
1110+
Relids required_outer = NULL;
1111+
ListCell *lc;
11101112

11111113
pathnode->path.pathtype = T_BitmapAnd;
11121114
pathnode->path.parent = rel;
11131115
pathnode->path.pathtarget = rel->reltarget;
1114-
pathnode->path.param_info = NULL; /* not used in bitmap trees */
1116+
1117+
/*
1118+
* Identify the required outer rels as the union of what the child paths
1119+
* depend on. (Alternatively, we could insist that the caller pass this
1120+
* in, but it's more convenient and reliable to compute it here.)
1121+
*/
1122+
foreach(lc, bitmapquals)
1123+
{
1124+
Path *bitmapqual = (Path *) lfirst(lc);
1125+
1126+
required_outer = bms_add_members(required_outer,
1127+
PATH_REQ_OUTER(bitmapqual));
1128+
}
1129+
pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
1130+
required_outer);
11151131

11161132
/*
11171133
* Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be
@@ -1143,11 +1159,27 @@ create_bitmap_or_path(PlannerInfo *root,
11431159
List *bitmapquals)
11441160
{
11451161
BitmapOrPath *pathnode = makeNode(BitmapOrPath);
1162+
Relids required_outer = NULL;
1163+
ListCell *lc;
11461164

11471165
pathnode->path.pathtype = T_BitmapOr;
11481166
pathnode->path.parent = rel;
11491167
pathnode->path.pathtarget = rel->reltarget;
1150-
pathnode->path.param_info = NULL; /* not used in bitmap trees */
1168+
1169+
/*
1170+
* Identify the required outer rels as the union of what the child paths
1171+
* depend on. (Alternatively, we could insist that the caller pass this
1172+
* in, but it's more convenient and reliable to compute it here.)
1173+
*/
1174+
foreach(lc, bitmapquals)
1175+
{
1176+
Path *bitmapqual = (Path *) lfirst(lc);
1177+
1178+
required_outer = bms_add_members(required_outer,
1179+
PATH_REQ_OUTER(bitmapqual));
1180+
}
1181+
pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
1182+
required_outer);
11511183

11521184
/*
11531185
* Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be
@@ -3851,7 +3883,18 @@ do { \
38513883
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
38523884
return path;
38533885

3854-
/* Reparameterize a copy of given path. */
3886+
/*
3887+
* If possible, reparameterize the given path, making a copy.
3888+
*
3889+
* This function is currently only applied to the inner side of a nestloop
3890+
* join that is being partitioned by the partitionwise-join code. Hence,
3891+
* we need only support path types that plausibly arise in that context.
3892+
* (In particular, supporting sorted path types would be a waste of code
3893+
* and cycles: even if we translated them here, they'd just lose in
3894+
* subsequent cost comparisons.) If we do see an unsupported path type,
3895+
* that just means we won't be able to generate a partitionwise-join plan
3896+
* using that path type.
3897+
*/
38553898
switch (nodeTag(path))
38563899
{
38573900
case T_Path:
@@ -3898,16 +3941,6 @@ do { \
38983941
}
38993942
break;
39003943

3901-
case T_TidPath:
3902-
{
3903-
TidPath *tpath;
3904-
3905-
FLAT_COPY_PATH(tpath, path, TidPath);
3906-
ADJUST_CHILD_ATTRS(tpath->tidquals);
3907-
new_path = (Path *) tpath;
3908-
}
3909-
break;
3910-
39113944
case T_ForeignPath:
39123945
{
39133946
ForeignPath *fpath;
@@ -3998,37 +4031,6 @@ do { \
39984031
}
39994032
break;
40004033

4001-
case T_MergeAppendPath:
4002-
{
4003-
MergeAppendPath *mapath;
4004-
4005-
FLAT_COPY_PATH(mapath, path, MergeAppendPath);
4006-
REPARAMETERIZE_CHILD_PATH_LIST(mapath->subpaths);
4007-
new_path = (Path *) mapath;
4008-
}
4009-
break;
4010-
4011-
case T_MaterialPath:
4012-
{
4013-
MaterialPath *mpath;
4014-
4015-
FLAT_COPY_PATH(mpath, path, MaterialPath);
4016-
REPARAMETERIZE_CHILD_PATH(mpath->subpath);
4017-
new_path = (Path *) mpath;
4018-
}
4019-
break;
4020-
4021-
case T_UniquePath:
4022-
{
4023-
UniquePath *upath;
4024-
4025-
FLAT_COPY_PATH(upath, path, UniquePath);
4026-
REPARAMETERIZE_CHILD_PATH(upath->subpath);
4027-
ADJUST_CHILD_ATTRS(upath->uniq_exprs);
4028-
new_path = (Path *) upath;
4029-
}
4030-
break;
4031-
40324034
case T_GatherPath:
40334035
{
40344036
GatherPath *gpath;
@@ -4039,16 +4041,6 @@ do { \
40394041
}
40404042
break;
40414043

4042-
case T_GatherMergePath:
4043-
{
4044-
GatherMergePath *gmpath;
4045-
4046-
FLAT_COPY_PATH(gmpath, path, GatherMergePath);
4047-
REPARAMETERIZE_CHILD_PATH(gmpath->subpath);
4048-
new_path = (Path *) gmpath;
4049-
}
4050-
break;
4051-
40524044
default:
40534045

40544046
/* We don't know how to reparameterize this path. */

0 commit comments

Comments
 (0)