On Thu, Feb 13, 2025 at 1:55 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Thu, Feb 13, 2025 at 8:44 AM Richard Guo <guofenglinux@gmail.com> wrote:
> > I'm thinking that a better approach might be to check if each member
> > of the child_sjinfo is a translated copy and only free it if that's
> > the case, something like attached.
>
> This approach doesn't allocate memory when translation is not required
> but it requires parent SJinfo to be present when
> free_child_join_sjinfo() is called. It's true right now but future
> changes to the code may not ensure that. So there is a potential
> maintenance overhead here.
Given the function's name, I don't see a problem with adding a new
parameter to pass parent_sjinfo. In fact, I'd be especially concerned
if any caller doesn’t have parent_sjinfo handy -- that could signal an
unexpected usage or future maintenance risk. Making it explicit helps
keep things correct and avoids silent errors.
> We have to be always cautious of freeing or modifying the Relids
> returned by adjust_child_relids() [1]. For example, we could free
> intermediate Relids in adjust_child_relids_multilevel() but we need to
> be cautious of them being same as the parent Relids. Amit's approach
> of bms_copy() in adjust_child_relids() frees us from that caution. May
> be we could pass a flag to adjust_child_relids() asking it to create a
> new bitmapset always. So even if it's a larger bandaid, it may be
> useful at multiple places.
We could do this separately, but for a bug fix, I'd prefer to keep the
changes focused on the issue at hand and avoid modifications unrelated
to the bug.
--
Thanks, Amit Langote
