On Thu, May 9, 2024 at 6:40 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> David Rowley <dgrowle...@gmail.com> writes: > > I'm fine with this one as it's the same as what I already mentioned > > earlier. I had imagined doing bms_del_member(bms_copy ... but maybe > > the compiler is able to optimise away the additional store. Likely, it > > does not matter much as pallocing memory likely adds far more overhead > > anyway. > > I actually wrote it that way to start with, but undid it after > noticing that the existing code in remove_rel_from_restrictinfo > does it in separate steps, and thinking that that was good for > both separation of concerns and a cleaner git history. I too > can't believe that an extra fetch will be noticeable compared > to the cost of the adjacent bms_xxx operations. I also think it seems better to do bms_copy in separate steps, not only because this keeps consistent with the existing code in remove_rel_from_restrictinfo, but also because we need to do bms_del_member twice for each lefthand/righthand relid set. Speaking of consistency, do you think it would improve the code's readability if we rearrange the code in remove_rel_from_query so that the modifications of the same relid set are grouped together, just like what we do in remove_rel_from_restrictinfo? I mean something like: sjinf->min_lefthand = bms_copy(sjinf->min_lefthand); sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand, relid); sjinf->min_lefthand = bms_del_member(sjinf->min_lefthand, ojrelid); ... Thanks Richard
