From: Alena Rybakina <a.rybakina@postgrespro.ru>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrei Lepikhov <lepihov@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Gregory Stark (as CFM) <stark.cfm@gmail.com>
Cc: Michał Kłeczek <michal@kleczek.org>
Cc: Dean Rasheed <dean.a.rasheed@gmail.com>
Subject: Re: Removing unneeded self joins
Date: Tue, 18 Feb 2025 10:24:02 +0300
Message-ID: <38e8cec8-bdef-4892-8e15-7f2207e5e157@postgrespro.ru> (raw)
In-Reply-To: <CAPpHfdseDr02BT+kFfW1ORWd0xnf9B=OUNmYkDxF2x-bE32UOw@mail.gmail.com>
References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru>
<CA+TgmoZ66x=uWasH+O40a=G=fpbXN1ANEdGVunvR8BKuekqa2g@mail.gmail.com>
<3622801.1715010885@sss.pgh.pa.us>
<CAPpHfduRB9oW3YRDpvPGcDSAbH4iu0LSRhxpRRAzBh7euC9amQ@mail.gmail.com>
<CAEZATCUeJ93gWCEzK1eRxyZakvv5PVG9LfF3PkSGMsNBC7h9kA@mail.gmail.com>
<CAPpHfdsj1WdqoCg+g4oq1XQ_=KOj0U4pXqhauBc7e+0msHu66A@mail.gmail.com>
<CAPpHfds16a7bjQHv-jCLMNzwdukJ_i3da2e0OJFB9L5rTCWEEg@mail.gmail.com>
<ac7d7334-7ebb-4330-a894-12ba12ffa4ae@gmail.com>
<CAPpHfdsuc524-BkXNZ+iz9dH=-KiU8qx9CSyE-Nip4Wjz5pGgg@mail.gmail.com>
<ac34e44e-f8e1-4601-8bb8-fa5e65578832@gmail.com>
<CAPpHfdvHzExyROoxvVQz6JR4tGVOjgNyHnu8xJmGcx-bMxWvqw@mail.gmail.com>
<5934ce1e-be02-4640-904c-f7fec3962784@gmail.com>
<CAPpHfdt9Z=iriS1Fj0Vyo41Z4cS+zUSZM8D3qXyw5LfHr_jVjA@mail.gmail.com>
<d6319b0c-b186-472f-8b64-41dfa41e7b5f@postgrespro.ru>
<CAPpHfdsFcde5VWCxSup7x_12ig6uynrFaOms62-M_c8DM-FTBQ@mail.gmail.com>
<CAPpHfdseDr02BT+kFfW1ORWd0xnf9B=OUNmYkDxF2x-bE32UOw@mail.gmail.com>
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@gmail.com> wrote:
>> On Tue, Feb 11, 2025 at 5:31 PM Alena Rybakina
>> <a.rybakina@postgrespro.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@gmail.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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.rybakina@postgrespro.ru, aekorotkov@gmail.com, pgsql-hackers@lists.postgresql.org, tgl@sss.pgh.pa.us, lepihov@gmail.com, robertmhaas@gmail.com, guofenglinux@gmail.com, exclusion@gmail.com, stark.cfm@gmail.com, michal@kleczek.org, dean.a.rasheed@gmail.com
Subject: Re: Removing unneeded self joins
In-Reply-To: <38e8cec8-bdef-4892-8e15-7f2207e5e157@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox