Re: Removing unneeded self joins

On 13.02.2025 02:00, Alexander Korotkov wrote:
> Thank you. I've integrated that into a patch. However, I've to
> change keep_rinfo_list to be passed by pointer to
> add_non_redundant_clauses(), because it might be changed in
> distribute_restrictinfo_to_rels(). Without that there is a case of
> duplicated clause in regression tests.
>
> I've changed 'inner' and 'outer' vise versa in
> remove_self_joins_one_group() for better readability (I believe that
> was discussed upthread but lost). Also, I did a round of improvement
> for comments and commit message.
On 15.02.2025 12:19, Alexander Korotkov wrote:
> On Thu, Feb 13, 2025 at 1:00 AM Alexander Korotkov<aekorotkov(at)gmail(dot)com> wrote:
>> On Tue, Feb 11, 2025 at 5:31 PM Alena Rybakina
>> <a(dot)rybakina(at)postgrespro(dot)ru> wrote:
>>> Hi! Thank you for the work with this subject, I think it is really
>>> important.
>>>
>>> On 10.02.2025 22:58, Alexander Korotkov wrote:
>>>> Hi!
>>>>
>>>> On Mon, Feb 10, 2025 at 7:19 AM Andrei Lepikhov<lepihov(at)gmail(dot)com> wrote:
>>>>> On 9/2/2025 18:41, Alexander Korotkov wrote:
>>>>>> Regarding adjust_relid_set() and replace_relid(). I think they are
>>>>>> now strictly equivalent, except for the case then old relid is given
>>>>>> and not found. In this case adjust_relid_set() returns the original
>>>>>> relids while replace_relid() returns a copy. The behavior of
>>>>>> adjust_relid_set() appears more desirable as we don't need extra
>>>>>> copying when no modification is done. So, I've replaced all
>>>>>> replace_relid() with adjust_relid_set().
>>>>> Ok, I glanced into it, and it makes sense to merge these routines.
>>>>> I think the comment to adjust_relid_set() should be arranged, too. See
>>>>> the attachment for a short variant of such modification.
>>>>>> Also, I did some grammar correction to your new comment in tests.
>>>>> Thanks!
>>>> I've further revised adjust_relid_set() header comment.
>>>>
>>>> Looking back to the work done since previous attempt to commit this to
>>>> pg17, I can highlight following.
>>>> 1) We're now using more of existing infrastructure including
>>>> adjust_relid_set() and ChangeVarNodes(). The most of complexity is
>>>> still there though.
>>>> 2) We've checked few ways to further simplify this patch. But yet the
>>>> current way still feels to be best possible.
>>>> 3) For sure, several bugs were fixed.
>>>>
>>>> I think we could give it another chance for pg18 after some further
>>>> polishing (at least commit message still needs to be revised). Any
>>>> thoughts on this? Tom?
>>>>
>>> I didn't find any mistakes, I just have a refactoring remark. I think
>>> the part where we add non-redundant expressions with the
>>> binfo_candidates, jinfo_candidates
>>> check can be moved to a separate function, otherwise the code is very
>>> repetitive in this place. I did it and attached diff file
>> Thank you. I've integrated that into a patch. However, I've to
>> change keep_rinfo_list to be passed by pointer to
>> add_non_redundant_clauses(), because it might be changed in
>> distribute_restrictinfo_to_rels(). Without that there is a case of
>> duplicated clause in regression tests.
>>
>> I've changed 'inner' and 'outer' vise versa in
>> remove_self_joins_one_group() for better readability (I believe that
>> was discussed upthread but lost). Also, I did a round of improvement
>> for comments and commit message.
> I've corrected some spelling error reported by Alexander Lakhin
> privately to me. Also, I've revised comments around ChangeVarNodes()
> and ChangeVarNodesExtended(). I'm going to continue nitpicking this
> patch during next couple days then push it if no objections.
I agree with your corrections and for me patch looks good.

--
Regards,
Alena Rybakina
Postgres Professional