Hi Richard, On Fri, Jul 28, 2023 at 2:03 PM Richard Guo <guofengli...@gmail.com> wrote: > > > It doesn't seem too expensive to translate SpecialJoinInfos, so I think > it's OK to construct and free the SpecialJoinInfo for a child join on > the fly. So the approach in 0002 looks reasonable to me. But there is > something that needs to be fixed in 0002.
Thanks for the review. I am fine if going ahead with 0002 is enough. > > + bms_free(child_sjinfo->commute_above_l); > + bms_free(child_sjinfo->commute_above_r); > + bms_free(child_sjinfo->commute_below_l); > + bms_free(child_sjinfo->commute_below_r); > > These four members in SpecialJoinInfo only contain outer join relids. > They do not need to be translated. So they would reference the same > memory areas in child_sjinfo as in parent_sjinfo. We should not free > them, otherwise they would become dangling pointers in parent sjinfo. > Good catch. Saved my time. I would have caught this rather hard way when running regression. Thanks a lot. I think we should 1. add an assert to make sure that commute_above_* do not require any transations i.e. they do not contain any parent relids of the child rels. 2. Do not free those. 3. Add a comment about keeping the build and free functions in sync. I will work on those next. -- Best Wishes, Ashutosh Bapat
