On Thu, Feb 13, 2025 at 4:25 PM Tender Wang <tndrwang@gmail.com> wrote:
> Amit Langote <amitlangote09@gmail.com> 于2025年2月13日周四 10:39写道：
>> On Wed, Feb 12, 2025 at 11:30 PM Richard Guo <guofenglinux@gmail.com> wrote:
>> > On Wed, Feb 12, 2025 at 10:04 PM Ashutosh Bapat
>> > <ashutosh.bapat.oss@gmail.com> wrote:
>> > > Hmm. The code there assumes that all the Relids will at least have one
>> > > parent each of the children involved. For some reason
>> > > sjinfo->min_lefthand has only relid 1 but not 2 or 5. 2 and 5 are
>> > > actually the parent relids of the children passed in respectively. The
>> > > join is between 2 and 5, then why is 1 appearing in the min_lefthand.
>> > > It might be legitimate, but we need to find the reason. If it's
>> > > legitimate, I think we need to copy the Relids which haven't undergone
>> > > any translation so as to keep them isolated from the parent relids.
>> >
>> > Yes, it's legitimate. For the semijoin, its join clause only
>> > references {1} and {5}, with no other ordering restrictions.
>> > Therefore, the minimum LHS for this join consists only of {1}.
>> >
>> > Instead of copying the untranslated Relids and freeing them later, I
>> > think it might be better to modify free_child_join_sjinfo() to avoid
>> > freeing the untranslated members of child_sjinfo.
>>
>> free_child_join_sjinfo() frees min_lefthand and other fields
>> initialized by build_child_join_sjinfo(), assuming that
>> adjust_child_relids() creates a copy. However, this does not always
>> seem to be the case, as demonstrated in this report.
>>
>> I'm wondering if the following line in adjust_child_relids() should be
>> using bms_copy() instead:
>>
>> /* Otherwise, return the original set without modification. */
>> return relids;
>> }
>
> This fix is ok for me. Keeping the child's info isolated from the parents seems safer.
I'm inclined to go with Richard's proposal, mainly to avoid the
overhead of copying in a bug fix just to isolate child sjinfo from
parent_sjinfo.
Richard, would you like to commit the patch you proposed (adding
parent_sjinfo as a parameter to free_child_join_sjinfo()), or would
you prefer that I do it?
--
Thanks, Amit Langote
