Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retire GUC parallel_hash_enable_motion_broadcast & fix potential Assertion failure #103

Merged
merged 2 commits into from
Oct 11, 2023
Merged

Retire GUC parallel_hash_enable_motion_broadcast & fix potential Assertion failure #103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bce6e30
Retire GUC parallel_hash_enable_motion_broadcast
avamingli Jul 30, 2023
3c3c227
Make BroadcastMotion if target plan's parallel < 1 when parallel aware.
avamingli Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions src/backend/cdb/cdbpath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2871,8 +2871,7 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
List *inner_pathkeys,
bool outer_require_existing_order,
bool inner_require_existing_order,
bool parallel_aware,
bool uninterested_broadcast)
bool parallel_aware)
{
CdbpathMfjRel outer;
CdbpathMfjRel inner;
Expand Down Expand Up @@ -3656,6 +3655,8 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
{ /* partitioned */
CdbpathMfjRel *large_rel = &outer;
CdbpathMfjRel *small_rel = &inner;
int lp; /* larger rel parallel workers */
int sp; /* small rel parallel workers */

/* Consider locus when parallel_ware. */
if(parallel_aware)
Expand All @@ -3671,6 +3672,9 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
if (large_rel->bytes < small_rel->bytes)
CdbSwap(CdbpathMfjRel *, large_rel, small_rel);

lp = CdbPathLocus_NumParallelWorkers(large_rel->locus);
sp = CdbPathLocus_NumParallelWorkers(small_rel->locus);

/* Both side are distribued in 1 segment and no parallel, it can join without motion. */
if (CdbPathLocus_NumSegments(large_rel->locus) == 1 &&
CdbPathLocus_NumSegments(small_rel->locus) == 1 &&
Expand Down Expand Up @@ -3699,9 +3703,9 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
else if (!small_rel->require_existing_order &&
small_rel->ok_to_replicate &&
((!parallel_aware && (small_rel->bytes * CdbPathLocus_NumSegmentsPlusParallelWorkers(large_rel->locus) < large_rel->bytes)) ||
(parallel_aware && !uninterested_broadcast && (small_rel->bytes * CdbPathLocus_NumSegments(large_rel->locus) < large_rel->bytes))))
(parallel_aware && (small_rel->bytes * CdbPathLocus_NumSegments(large_rel->locus) < large_rel->bytes))))
{
if (!parallel_aware)
if (!parallel_aware || lp <= 1)
CdbPathLocus_MakeReplicated(&small_rel->move_to,
CdbPathLocus_NumSegments(large_rel->locus),
large_rel->path->parallel_workers);
Expand All @@ -3718,9 +3722,9 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
else if (!large_rel->require_existing_order &&
large_rel->ok_to_replicate &&
((!parallel_aware && (large_rel->bytes * CdbPathLocus_NumSegmentsPlusParallelWorkers(small_rel->locus) < small_rel->bytes)) ||
(parallel_aware && !uninterested_broadcast && (large_rel->bytes * CdbPathLocus_NumSegments(small_rel->locus) < small_rel->bytes))))
(parallel_aware && (large_rel->bytes * CdbPathLocus_NumSegments(small_rel->locus) < small_rel->bytes))))
{
if (!parallel_aware)
if (!parallel_aware || sp <= 1)
CdbPathLocus_MakeReplicated(&large_rel->move_to,
CdbPathLocus_NumSegments(small_rel->locus),
small_rel->path->parallel_workers);
Expand Down Expand Up @@ -3749,9 +3753,9 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
else if (!small_rel->require_existing_order &&
small_rel->ok_to_replicate &&
((!parallel_aware && (small_rel->bytes * CdbPathLocus_NumSegmentsPlusParallelWorkers(large_rel->locus) < small_rel->bytes + large_rel->bytes)) ||
(parallel_aware && !uninterested_broadcast && (small_rel->bytes * CdbPathLocus_NumSegments(large_rel->locus) < small_rel->bytes + large_rel->bytes))))
(parallel_aware && (small_rel->bytes * CdbPathLocus_NumSegments(large_rel->locus) < small_rel->bytes + large_rel->bytes))))
{
if (!parallel_aware)
if (!parallel_aware || lp <= 1)
CdbPathLocus_MakeReplicated(&small_rel->move_to,
CdbPathLocus_NumSegments(large_rel->locus),
large_rel->path->parallel_workers);
Expand All @@ -3765,9 +3769,9 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
else if (!large_rel->require_existing_order &&
large_rel->ok_to_replicate &&
((!parallel_aware && (large_rel->bytes * CdbPathLocus_NumSegmentsPlusParallelWorkers(small_rel->locus) < small_rel->bytes + large_rel->bytes)) ||
(parallel_aware && !uninterested_broadcast && (large_rel->bytes * CdbPathLocus_NumSegments(small_rel->locus) < small_rel->bytes + large_rel->bytes))))
(parallel_aware && (large_rel->bytes * CdbPathLocus_NumSegments(small_rel->locus) < small_rel->bytes + large_rel->bytes))))
{
if (!parallel_aware)
if (!parallel_aware || sp <= 1)
CdbPathLocus_MakeReplicated(&large_rel->move_to,
CdbPathLocus_NumSegments(small_rel->locus),
small_rel->path->parallel_workers);
Expand Down Expand Up @@ -3809,7 +3813,7 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
else if (!small_rel->require_existing_order &&
small_rel->ok_to_replicate)
{
if (!parallel_aware)
if (!parallel_aware || lp <= 1)
CdbPathLocus_MakeReplicated(&small_rel->move_to,
CdbPathLocus_NumSegments(large_rel->locus),
large_rel->path->parallel_workers);
Expand All @@ -3822,7 +3826,7 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
else if (!large_rel->require_existing_order &&
large_rel->ok_to_replicate)
{
if (!parallel_aware)
if (!parallel_aware || sp <= 1)
CdbPathLocus_MakeReplicated(&large_rel->move_to,
CdbPathLocus_NumSegments(small_rel->locus),
small_rel->path->parallel_workers);
Expand Down
41 changes: 3 additions & 38 deletions src/backend/optimizer/path/joinpath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1089,8 +1089,7 @@ try_hashjoin_path(PlannerInfo *root,
extra->restrictlist,
required_outer,
extra->redistribution_clauses,
hashclauses,
false),
hashclauses),
root);
}
else
Expand Down Expand Up @@ -1147,40 +1146,6 @@ try_partial_hashjoin_path(PlannerInfo *root,
if (!add_partial_path_precheck(joinrel, workspace.total_cost, NIL))
return;

/*
* CBDB_PARALLEL_FIXME
* Customers encounter an issue that when parallel hash, broadcast motion
* a smaller table may be worser than redistribute a big table.
* We add a path whic doesn't try broadcast if possible.
* And let the path cost decide which is better.
*/
if (parallel_hash)
{
hashpath = create_hashjoin_path(root,
joinrel,
jointype,
orig_jointype,
&workspace,
extra,
outer_path,
inner_path,
true,
extra->restrictlist,
NULL,
extra->redistribution_clauses,
hashclauses,
true); /* not use broadcast */
if (hashpath && hashpath->parallel_safe)
add_partial_path(joinrel, hashpath);
}

/*
* CBDB_PARALLEL_FIXME:
* We only want non-broadcast in parallel hash if the guc is set.
*/
if (parallel_hash && !parallel_hash_enable_motion_broadcast)
return;

hashpath = create_hashjoin_path(root,
joinrel,
jointype,
Expand All @@ -1193,8 +1158,8 @@ try_partial_hashjoin_path(PlannerInfo *root,
extra->restrictlist,
NULL,
extra->redistribution_clauses,
hashclauses,
false);
hashclauses);

/* Might be good enough to be worth trying and no motion, so let's try it. */
if (hashpath && hashpath->parallel_safe)
add_partial_path(joinrel, hashpath);
Expand Down
8 changes: 2 additions & 6 deletions src/backend/optimizer/util/pathnode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3911,7 +3911,6 @@ create_nestloop_path(PlannerInfo *root,
NIL,
outer_must_be_local,
inner_must_be_local,
false,
false);
}

Expand Down Expand Up @@ -4191,7 +4190,6 @@ create_mergejoin_path(PlannerInfo *root,
innermotionkeys,
preserve_outer_ordering,
preserve_inner_ordering,
false,
false);
}

Expand Down Expand Up @@ -4332,8 +4330,7 @@ create_hashjoin_path(PlannerInfo *root,
List *restrict_clauses,
Relids required_outer,
List *redistribution_clauses, /* CDB */
List *hashclauses,
bool uninterested_broadcast) /* GPDB parallel */
List *hashclauses)
{
HashPath *pathnode;
CdbPathLocus join_locus;
Expand Down Expand Up @@ -4378,8 +4375,7 @@ create_hashjoin_path(PlannerInfo *root,
NIL,
outer_must_be_local,
inner_must_be_local,
parallel_hash,
uninterested_broadcast);
parallel_hash);
}

if (CdbPathLocus_IsNull(join_locus))
Expand Down
11 changes: 0 additions & 11 deletions src/backend/utils/misc/guc_gp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,6 @@ bool optimizer_enable_indexjoin;
bool optimizer_enable_motions_masteronly_queries;
bool optimizer_enable_motions;
bool optimizer_enable_motion_broadcast;
bool parallel_hash_enable_motion_broadcast;
bool optimizer_enable_motion_gather;
bool optimizer_enable_motion_redistribute;
bool optimizer_enable_sort;
Expand Down Expand Up @@ -2090,16 +2089,6 @@ struct config_bool ConfigureNamesBool_gp[] =
true,
NULL, NULL, NULL
},
{
{"parallel_hash_enable_motion_broadcast", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Enable plans with Motion Broadcast operators in parallel hash join."),
NULL,
GUC_NO_SHOW_ALL | GUC_NOT_IN_SAMPLE
},
&parallel_hash_enable_motion_broadcast,
true,
NULL, NULL, NULL
},
{
{"optimizer_enable_motion_gather", PGC_USERSET, DEVELOPER_OPTIONS,
gettext_noop("Enable plans with Motion Gather operators in the optimizer."),
Expand Down
3 changes: 1 addition & 2 deletions src/include/cdb/cdbpath.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ cdbpath_motion_for_parallel_join(PlannerInfo *root,
List *inner_pathkeys,
bool outer_require_existing_order,
bool inner_require_existing_order,
bool parallel_aware,
bool uninterested_broadcast); /* for parallel hash join, do not use Broadcast if possible */
bool parallel_aware);

#endif /* CDBPATH_H */
3 changes: 1 addition & 2 deletions src/include/optimizer/pathnode.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,7 @@ extern Path *create_hashjoin_path(PlannerInfo *root,
List *restrict_clauses,
Relids required_outer,
List *redistribution_clauses, /*CDB*/
List *hashclauses,
bool uninterested_broadcast); /* GPDB parallel */
List *hashclauses);

extern ProjectionPath *create_projection_path(PlannerInfo *root,
RelOptInfo *rel,
Expand Down
1 change: 0 additions & 1 deletion src/include/utils/guc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -496,7 +496,6 @@ extern bool optimizer_enable_indexjoin;
extern bool optimizer_enable_motions_masteronly_queries;
extern bool optimizer_enable_motions;
extern bool optimizer_enable_motion_broadcast;
extern bool parallel_hash_enable_motion_broadcast;
extern bool optimizer_enable_motion_gather;
extern bool optimizer_enable_motion_redistribute;
extern bool optimizer_enable_sort;
Expand Down
1 change: 0 additions & 1 deletion src/include/utils/unsync_guc_name.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,6 @@
"optimizer_enable_materialize",
"optimizer_enable_mergejoin",
"optimizer_enable_motion_broadcast",
"parallel_hash_enable_motion_broadcast",
"optimizer_enable_motion_gather",
"optimizer_enable_motion_redistribute",
"optimizer_enable_motions",
Expand Down