On Wed, Sep 27, 2023 at 8:07 PM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > On Wed, Sep 27, 2023 at 2:30 PM Amit Langote <amitlangot...@gmail.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