From 9d8c408b3379faab6269421f67d75530f1c4cde2 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Fri, 22 Mar 2024 16:51:48 +0900 Subject: [PATCH v1 2/2] Some fixes * Revert (IMO) unnecessary modifications of build_child_join_sjinfo(). For example, hunks to rename sjinfo to child_sjinfo seem unnecessary, because it's clear from the context that they are "child" sjinfos. * Rename free_child_sjinfo_members() to free_child_join_sjinfo() to be symmetric with build_child_join_sjinfo(). Note that the function is charged with freeing the sjinfo itself too. Like build_child_join_sjinfo(), name the argument sjinfo, not child_sjinfo. * Don't set the child sjinfo pointer to NULL after freeing it. While a good convention in general, it's not really common in the planner code, so doing it here seems a bit overkill * Rename make_dummy_sjinfo() to init_dummy_sjinfo() because the function is not really making it per se, only initializing fields in the SpecialJoinInfo struct made available by the caller. * Make init_dummy_sjinfo() take Relids instead of RelOptInfos because that's what's needed. Also allows to reduce changes to build_child_join_sjinfo()'s signature. * Update the reason mentioned in a comment in free_child_join_sjinfo() about why semi_rhs_expr is not freed -- it may be freed, but there's no way to "deep" free it, so we leave it alone. * Update a comment somewhere about why SpecialJoinInfo can be "transient" sometimes. --- src/backend/optimizer/path/costsize.c | 6 +- src/backend/optimizer/path/joinrels.c | 125 +++++++++++++------------- src/backend/optimizer/util/pathnode.c | 5 +- src/include/optimizer/paths.h | 4 +- 4 files changed, 70 insertions(+), 70 deletions(-) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 1b76523e79..ee23ed7835 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -5050,7 +5050,7 @@ compute_semi_anti_join_factors(PlannerInfo *root, /* * Also get the normal inner-join selectivity of the join clauses. */ - make_dummy_sjinfo(&norm_sjinfo, outerrel, innerrel); + init_dummy_sjinfo(&norm_sjinfo, outerrel->relids, innerrel->relids); nselec = clauselist_selectivity(root, joinquals, @@ -5203,8 +5203,8 @@ approx_tuple_count(PlannerInfo *root, JoinPath *path, List *quals) /* * Make up a SpecialJoinInfo for JOIN_INNER semantics. */ - make_dummy_sjinfo(&sjinfo, path->outerjoinpath->parent, - path->innerjoinpath->parent); + init_dummy_sjinfo(&sjinfo, path->outerjoinpath->parent->relids, + path->innerjoinpath->parent->relids); /* Get the approximate selectivity */ foreach(l, quals) diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c index 465dcf7d5d..bef5b5f213 100644 --- a/src/backend/optimizer/path/joinrels.c +++ b/src/backend/optimizer/path/joinrels.c @@ -44,9 +44,8 @@ static void try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, List *parent_restrictlist); static SpecialJoinInfo *build_child_join_sjinfo(PlannerInfo *root, SpecialJoinInfo *parent_sjinfo, - RelOptInfo *left_child, - RelOptInfo *right_child); -static void free_child_sjinfo_members(SpecialJoinInfo **child_sjinfo); + Relids left_relids, Relids right_relids); +static void free_child_join_sjinfo(SpecialJoinInfo *child_sjinfo); static void compute_partition_bounds(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, RelOptInfo *joinrel, SpecialJoinInfo *parent_sjinfo, @@ -656,18 +655,24 @@ join_is_legal(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, } /* - * make_dummy_sjinfo - * Populate the given SpecialJoinInfo for a plain inner join, between rel1 - * and rel2, which does not have a SpecialJoinInfo associated with it. + * init_dummy_sjinfo + * Populate the given SpecialJoinInfo for a plain inner join between rel1 + * and rel2 + * + * Inner joins do not normally have an associated SpecialJoinInfo node. But + * some functions involved in join planning require one to be passed, + * containing at least the information of which relations are being joined, + * so we initialize that information here. */ void -make_dummy_sjinfo(SpecialJoinInfo *sjinfo, RelOptInfo *rel1, RelOptInfo *rel2) +init_dummy_sjinfo(SpecialJoinInfo *sjinfo, Relids left_relids, + Relids right_relids) { sjinfo->type = T_SpecialJoinInfo; - sjinfo->min_lefthand = rel1->relids; - sjinfo->min_righthand = rel2->relids; - sjinfo->syn_lefthand = rel1->relids; - sjinfo->syn_righthand = rel2->relids; + sjinfo->min_lefthand = left_relids; + sjinfo->min_righthand = right_relids; + sjinfo->syn_lefthand = left_relids; + sjinfo->syn_righthand = right_relids; sjinfo->jointype = JOIN_INNER; sjinfo->ojrelid = 0; sjinfo->commute_above_l = NULL; @@ -744,7 +749,7 @@ make_join_rel(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2) if (sjinfo == NULL) { sjinfo = &sjinfo_data; - make_dummy_sjinfo(sjinfo, rel1, rel2); + init_dummy_sjinfo(sjinfo, rel1->relids, rel2->relids); } /* @@ -1628,8 +1633,9 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, * Construct SpecialJoinInfo from parent join relations's * SpecialJoinInfo. */ - child_sjinfo = build_child_join_sjinfo(root, parent_sjinfo, child_rel1, - child_rel2); + child_sjinfo = build_child_join_sjinfo(root, parent_sjinfo, + child_rel1->relids, + child_rel2->relids); /* Find the AppendRelInfo structures */ appinfos = find_appinfos_by_relids(root, @@ -1670,7 +1676,7 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, child_restrictlist); pfree(appinfos); - free_child_sjinfo_members(&child_sjinfo); + free_child_join_sjinfo(child_sjinfo); } } @@ -1679,91 +1685,84 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, * SpecialJoinInfo for the join between parents. left_relids and right_relids * are the relids of left and right side of the join respectively. * - * If translations are added to or removed from this function, consider freeing - * the translated objects in free_child_sjinfo_members() appropriately. + * If translations are added to or removed from this function, consider + * updating free_child_join_sjinfo() appropriately. */ static SpecialJoinInfo * build_child_join_sjinfo(PlannerInfo *root, SpecialJoinInfo *parent_sjinfo, - RelOptInfo *left_child, RelOptInfo *right_child) + Relids left_relids, Relids right_relids) { + SpecialJoinInfo *sjinfo = makeNode(SpecialJoinInfo); AppendRelInfo **left_appinfos; int left_nappinfos; AppendRelInfo **right_appinfos; int right_nappinfos; - SpecialJoinInfo *child_sjinfo = makeNode(SpecialJoinInfo); /* Dummy SpecialJoinInfos can be created without any translation. */ if (parent_sjinfo->jointype == JOIN_INNER) { Assert(parent_sjinfo->ojrelid == 0); - make_dummy_sjinfo(child_sjinfo, left_child, right_child); - return child_sjinfo; + init_dummy_sjinfo(sjinfo, left_relids, right_relids); + return sjinfo; } - memcpy(child_sjinfo, parent_sjinfo, sizeof(SpecialJoinInfo)); - left_appinfos = find_appinfos_by_relids(root, left_child->relids, + memcpy(sjinfo, parent_sjinfo, sizeof(SpecialJoinInfo)); + left_appinfos = find_appinfos_by_relids(root, left_relids, &left_nappinfos); - right_appinfos = find_appinfos_by_relids(root, right_child->relids, + right_appinfos = find_appinfos_by_relids(root, right_relids, &right_nappinfos); - child_sjinfo->min_lefthand = - adjust_child_relids(child_sjinfo->min_lefthand, - left_nappinfos, - left_appinfos); - child_sjinfo->min_righthand = - adjust_child_relids(child_sjinfo->min_righthand, - right_nappinfos, - right_appinfos); - child_sjinfo->syn_lefthand = adjust_child_relids(child_sjinfo->syn_lefthand, - left_nappinfos, - left_appinfos); - child_sjinfo->syn_righthand = adjust_child_relids(child_sjinfo->syn_righthand, - right_nappinfos, - right_appinfos); - + sjinfo->min_lefthand = adjust_child_relids(sjinfo->min_lefthand, + left_nappinfos, left_appinfos); + sjinfo->min_righthand = adjust_child_relids(sjinfo->min_righthand, + right_nappinfos, + right_appinfos); + sjinfo->syn_lefthand = adjust_child_relids(sjinfo->syn_lefthand, + left_nappinfos, left_appinfos); + sjinfo->syn_righthand = adjust_child_relids(sjinfo->syn_righthand, + right_nappinfos, + right_appinfos); /* outer-join relids need no adjustment */ - child_sjinfo->semi_rhs_exprs = - (List *) adjust_appendrel_attrs(root, - (Node *) child_sjinfo->semi_rhs_exprs, - right_nappinfos, right_appinfos); + sjinfo->semi_rhs_exprs = (List *) adjust_appendrel_attrs(root, + (Node *) sjinfo->semi_rhs_exprs, + right_nappinfos, + right_appinfos); pfree(left_appinfos); pfree(right_appinfos); - return child_sjinfo; + return sjinfo; } /* - * free_child_sjinfo_members - * Free memory consumed by a child SpecialJoinInfo. + * free_child_join_sjinfo + * Free memory consumed by a SpecialJoinInfo created by + * build_child_join_sjinfo() * * Only members that are translated copies of their counterpart in the parent - * SpecialJoinInfo are freed here. However, members that could be referenced - * elsewhere are not freed. + * SpecialJoinInfo are freed here; see build_child_join_sjinfo(). */ static void -free_child_sjinfo_members(SpecialJoinInfo **child_sjinfo_p) +free_child_join_sjinfo(SpecialJoinInfo *sjinfo) { - SpecialJoinInfo *child_sjinfo = *child_sjinfo_p; - /* - * Dummy SpecialJoinInfos do not have any translated fields and hence have - * nothing to free. + * Dummy SpecialJoinInfos of inner joins do not have any translated fields + * and hence have nothing to be freed; see init_dummy_sjinfo(). */ - if (child_sjinfo->jointype != JOIN_INNER) + if (sjinfo->jointype != JOIN_INNER) { - bms_free(child_sjinfo->min_lefthand); - bms_free(child_sjinfo->min_righthand); - bms_free(child_sjinfo->syn_lefthand); - bms_free(child_sjinfo->syn_righthand); + bms_free(sjinfo->min_lefthand); + bms_free(sjinfo->min_righthand); + bms_free(sjinfo->syn_lefthand); + bms_free(sjinfo->syn_righthand); - /* semi_rhs_exprs may be referenced, so don't free. */ + /* + * semi_rhs_exprs may in principle be freed, but a simple pfree() does + * not suffice, so we leave it alone. + */ } - /* Avoid dangling pointer. */ - *child_sjinfo_p = NULL; - - pfree(child_sjinfo); + pfree(sjinfo); } /* diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 0a7e5c2678..c29ca5a0da 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1707,8 +1707,9 @@ create_unique_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath, pathnode->subpath = subpath; /* - * Under GEQO, the sjinfo might be short-lived, so we'd better make copies - * of data structures we extract from it. + * Under GEQO and when planning child joins, the sjinfo might be + * short-lived, so we'd better make copies of data structures we extract + * from it. */ pathnode->in_operators = copyObject(sjinfo->semi_operators); pathnode->uniq_exprs = copyObject(sjinfo->semi_rhs_exprs); diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index 1e340a6335..125676b5bf 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -112,8 +112,8 @@ extern bool have_join_order_restriction(PlannerInfo *root, extern bool have_dangerous_phv(PlannerInfo *root, Relids outer_relids, Relids inner_params); extern void mark_dummy_rel(RelOptInfo *rel); -extern void make_dummy_sjinfo(SpecialJoinInfo *sjinfo, RelOptInfo *rel1, - RelOptInfo *rel2); +extern void init_dummy_sjinfo(SpecialJoinInfo *sjinfo, Relids left_relids, + Relids right_relids); /* * equivclass.c -- 2.43.0