Re: Memory consumed by child SpecialJoinInfo in partitionwise join planning

On Wed, Sep 27, 2023 at 8:07 PM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
> On Wed, Sep 27, 2023 at 2:30 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > + /*
> > + * But the list of operator OIDs and the list of expressions may be
> > + * referenced somewhere else. Do not free those.
> > + */
> >
> > I don't see build_child_join_sjinfo() copying any list of OIDs, so I'm
> > not sure what the comment is referring to. Also, instead of "the list
> > of expressions", it might be more useful to mention semi_rhs_expr,
> > because that's the only one being copied.
>
> list of OID is semi_operators. It's copied as is from parent
> SpecialJoinInfo. But the way it's mentioned in the comment is
> confusing. I have modified the prologue of function to provide a
> general guidance on what can be freed and what should not be. and then
> specific examples. Let me know if this is more clear.

Thanks. I would've kept the notes about specific members inside the
function. Also, no need to have a note for pointers that are not
deep-copied to begin with, such as semi_operators. IOW, something
like the following would have sufficed:

@@ -1735,6 +1735,10 @@ build_child_join_sjinfo(PlannerInfo *root,
SpecialJoinInfo *parent_sjinfo,
/*
* free_child_sjinfo_members
* Free memory consumed by members of a child SpecialJoinInfo.
+ *
+ * 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.
*/
static void
free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
@@ -1755,10 +1759,7 @@ free_child_sjinfo_members(SpecialJoinInfo *child_sjinfo)
bms_free(child_sjinfo->syn_lefthand);
bms_free(child_sjinfo->syn_righthand);

- /*
- * But the list of operator OIDs and the list of expressions may be
- * referenced somewhere else. Do not free those.
- */
+ /* semi_rhs_exprs may be referenced, so don't free. */
}

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com