Skip to content

Commit e9f0311

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 d2761b6 commit e9f0311

File tree

4 files changed

+218
-162
lines changed

4 files changed

+218
-162
lines changed

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

Lines changed: 17 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,6 @@ static Cost bitmap_and_cost_est(PlannerInfo *root, RelOptInfo *rel,
126126
List *paths);
127127
static PathClauseUsage *classify_index_clause_usage(Path *path,
128128
List **clauselist);
129-
static Relids get_bitmap_tree_required_outer(Path *bitmapqual);
130129
static void find_indexpath_quals(Path *bitmapqual, List **quals, List **preds);
131130
static int find_list_position(Node *node, List **nodelist);
132131
static bool check_index_only(RelOptInfo *rel, IndexOptInfo *index);
@@ -356,23 +355,16 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
356355
*/
357356
if (bitjoinpaths != NIL)
358357
{
359-
List *path_outer;
360358
List *all_path_outers;
361359
ListCell *lc;
362360

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

374-
required_outer = get_bitmap_tree_required_outer(path);
375-
path_outer = lappend(path_outer, required_outer);
376368
if (!bms_equal_any(required_outer, all_path_outers))
377369
all_path_outers = lappend(all_path_outers, required_outer);
378370
}
@@ -387,16 +379,14 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
387379
double loop_count;
388380
BitmapHeapPath *bpath;
389381
ListCell *lcp;
390-
ListCell *lco;
391382

392383
/* Identify all the bitmap join paths needing no more than that */
393384
this_path_set = NIL;
394-
forboth(lcp, bitjoinpaths, lco, path_outer)
385+
foreach(lcp, bitjoinpaths)
395386
{
396387
Path *path = (Path *) lfirst(lcp);
397-
Relids p_outers = (Relids) lfirst(lco);
398388

399-
if (bms_is_subset(p_outers, max_outers))
389+
if (bms_is_subset(PATH_REQ_OUTER(path), max_outers))
400390
this_path_set = lappend(this_path_set, path);
401391
}
402392

@@ -410,7 +400,7 @@ create_index_paths(PlannerInfo *root, RelOptInfo *rel)
410400
bitmapqual = choose_bitmap_and(root, rel, this_path_set);
411401

412402
/* And push that path into the mix */
413-
required_outer = get_bitmap_tree_required_outer(bitmapqual);
403+
required_outer = PATH_REQ_OUTER(bitmapqual);
414404
loop_count = get_loop_count(root, rel->relid, required_outer);
415405
bpath = create_bitmap_heap_path(root, rel, bitmapqual,
416406
required_outer, loop_count, 0);
@@ -1611,25 +1601,19 @@ path_usage_comparator(const void *a, const void *b)
16111601

16121602
/*
16131603
* Estimate the cost of actually executing a bitmap scan with a single
1614-
* index path (no BitmapAnd, at least not at this level; but it could be
1615-
* a BitmapOr).
1604+
* index path (which could be a BitmapAnd or BitmapOr node).
16161605
*/
16171606
static Cost
16181607
bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
16191608
{
16201609
BitmapHeapPath bpath;
1621-
Relids required_outer;
1622-
1623-
/* Identify required outer rels, in case it's a parameterized scan */
1624-
required_outer = get_bitmap_tree_required_outer(ipath);
16251610

16261611
/* Set up a dummy BitmapHeapPath */
16271612
bpath.path.type = T_BitmapHeapPath;
16281613
bpath.path.pathtype = T_BitmapHeapScan;
16291614
bpath.path.parent = rel;
16301615
bpath.path.pathtarget = rel->reltarget;
1631-
bpath.path.param_info = get_baserel_parampathinfo(root, rel,
1632-
required_outer);
1616+
bpath.path.param_info = ipath->param_info;
16331617
bpath.path.pathkeys = NIL;
16341618
bpath.bitmapqual = ipath;
16351619

@@ -1638,10 +1622,13 @@ bitmap_scan_cost_est(PlannerInfo *root, RelOptInfo *rel, Path *ipath)
16381622
* Parallel bitmap heap path will be considered at later stage.
16391623
*/
16401624
bpath.path.parallel_workers = 0;
1625+
1626+
/* Now we can do cost_bitmap_heap_scan */
16411627
cost_bitmap_heap_scan(&bpath.path, root, rel,
16421628
bpath.path.param_info,
16431629
ipath,
1644-
get_loop_count(root, rel->relid, required_outer));
1630+
get_loop_count(root, rel->relid,
1631+
PATH_REQ_OUTER(ipath)));
16451632

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

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

1695-
return bpath.path.total_cost;
1651+
return bitmap_scan_cost_est(root, rel, (Path *) apath);
16961652
}
16971653

16981654

@@ -1763,49 +1719,6 @@ classify_index_clause_usage(Path *path, List **clauselist)
17631719
}
17641720

17651721

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

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

Lines changed: 46 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -1118,11 +1118,27 @@ create_bitmap_and_path(PlannerInfo *root,
11181118
List *bitmapquals)
11191119
{
11201120
BitmapAndPath *pathnode = makeNode(BitmapAndPath);
1121+
Relids required_outer = NULL;
1122+
ListCell *lc;
11211123

11221124
pathnode->path.pathtype = T_BitmapAnd;
11231125
pathnode->path.parent = rel;
11241126
pathnode->path.pathtarget = rel->reltarget;
1125-
pathnode->path.param_info = NULL; /* not used in bitmap trees */
1127+
1128+
/*
1129+
* Identify the required outer rels as the union of what the child paths
1130+
* depend on. (Alternatively, we could insist that the caller pass this
1131+
* in, but it's more convenient and reliable to compute it here.)
1132+
*/
1133+
foreach(lc, bitmapquals)
1134+
{
1135+
Path *bitmapqual = (Path *) lfirst(lc);
1136+
1137+
required_outer = bms_add_members(required_outer,
1138+
PATH_REQ_OUTER(bitmapqual));
1139+
}
1140+
pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
1141+
required_outer);
11261142

11271143
/*
11281144
* Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be
@@ -1154,11 +1170,27 @@ create_bitmap_or_path(PlannerInfo *root,
11541170
List *bitmapquals)
11551171
{
11561172
BitmapOrPath *pathnode = makeNode(BitmapOrPath);
1173+
Relids required_outer = NULL;
1174+
ListCell *lc;
11571175

11581176
pathnode->path.pathtype = T_BitmapOr;
11591177
pathnode->path.parent = rel;
11601178
pathnode->path.pathtarget = rel->reltarget;
1161-
pathnode->path.param_info = NULL; /* not used in bitmap trees */
1179+
1180+
/*
1181+
* Identify the required outer rels as the union of what the child paths
1182+
* depend on. (Alternatively, we could insist that the caller pass this
1183+
* in, but it's more convenient and reliable to compute it here.)
1184+
*/
1185+
foreach(lc, bitmapquals)
1186+
{
1187+
Path *bitmapqual = (Path *) lfirst(lc);
1188+
1189+
required_outer = bms_add_members(required_outer,
1190+
PATH_REQ_OUTER(bitmapqual));
1191+
}
1192+
pathnode->path.param_info = get_baserel_parampathinfo(root, rel,
1193+
required_outer);
11621194

11631195
/*
11641196
* Currently, a BitmapHeapPath, BitmapAndPath, or BitmapOrPath will be
@@ -3686,7 +3718,18 @@ do { \
36863718
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
36873719
return path;
36883720

3689-
/* Reparameterize a copy of given path. */
3721+
/*
3722+
* If possible, reparameterize the given path, making a copy.
3723+
*
3724+
* This function is currently only applied to the inner side of a nestloop
3725+
* join that is being partitioned by the partitionwise-join code. Hence,
3726+
* we need only support path types that plausibly arise in that context.
3727+
* (In particular, supporting sorted path types would be a waste of code
3728+
* and cycles: even if we translated them here, they'd just lose in
3729+
* subsequent cost comparisons.) If we do see an unsupported path type,
3730+
* that just means we won't be able to generate a partitionwise-join plan
3731+
* using that path type.
3732+
*/
36903733
switch (nodeTag(path))
36913734
{
36923735
case T_Path:
@@ -3734,20 +3777,6 @@ do { \
37343777
}
37353778
break;
37363779

3737-
case T_TidPath:
3738-
{
3739-
TidPath *tpath;
3740-
3741-
/*
3742-
* TidPath contains tidquals, which do not contain any
3743-
* external parameters per create_tidscan_path(). So don't
3744-
* bother to translate those.
3745-
*/
3746-
FLAT_COPY_PATH(tpath, path, TidPath);
3747-
new_path = (Path *) tpath;
3748-
}
3749-
break;
3750-
37513780
case T_ForeignPath:
37523781
{
37533782
ForeignPath *fpath;
@@ -3838,37 +3867,6 @@ do { \
38383867
}
38393868
break;
38403869

3841-
case T_MergeAppendPath:
3842-
{
3843-
MergeAppendPath *mapath;
3844-
3845-
FLAT_COPY_PATH(mapath, path, MergeAppendPath);
3846-
REPARAMETERIZE_CHILD_PATH_LIST(mapath->subpaths);
3847-
new_path = (Path *) mapath;
3848-
}
3849-
break;
3850-
3851-
case T_MaterialPath:
3852-
{
3853-
MaterialPath *mpath;
3854-
3855-
FLAT_COPY_PATH(mpath, path, MaterialPath);
3856-
REPARAMETERIZE_CHILD_PATH(mpath->subpath);
3857-
new_path = (Path *) mpath;
3858-
}
3859-
break;
3860-
3861-
case T_UniquePath:
3862-
{
3863-
UniquePath *upath;
3864-
3865-
FLAT_COPY_PATH(upath, path, UniquePath);
3866-
REPARAMETERIZE_CHILD_PATH(upath->subpath);
3867-
ADJUST_CHILD_ATTRS(upath->uniq_exprs);
3868-
new_path = (Path *) upath;
3869-
}
3870-
break;
3871-
38723870
case T_GatherPath:
38733871
{
38743872
GatherPath *gpath;
@@ -3879,16 +3877,6 @@ do { \
38793877
}
38803878
break;
38813879

3882-
case T_GatherMergePath:
3883-
{
3884-
GatherMergePath *gmpath;
3885-
3886-
FLAT_COPY_PATH(gmpath, path, GatherMergePath);
3887-
REPARAMETERIZE_CHILD_PATH(gmpath->subpath);
3888-
new_path = (Path *) gmpath;
3889-
}
3890-
break;
3891-
38923880
default:
38933881

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

0 commit comments

Comments
 (0)