Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tkHxh-002LiV-G0 for pgsql-hackers@arkaria.postgresql.org; Tue, 18 Feb 2025 07:24:10 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tkHxg-007dV4-69 for pgsql-hackers@arkaria.postgresql.org; Tue, 18 Feb 2025 07:24:08 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tkHxf-007dUw-Lp for pgsql-hackers@lists.postgresql.org; Tue, 18 Feb 2025 07:24:07 +0000 Received: from mail.postgrespro.ru ([93.174.131.139]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1tkHxd-001Vna-1N for pgsql-hackers@lists.postgresql.org; Tue, 18 Feb 2025 07:24:07 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=postgrespro.ru; s=mx2023; t=1739863444; bh=H5J3R0VXXEFOSnBhVHYzuwHdPbdSJH4myR0/6fI3gsk=; h=Message-ID:Date:User-Agent:Subject:To:Cc:References:From: In-Reply-To:From; b=3ngRLX6LzWj1AoFDhF/Z4EzWw3RqtfLiU21126uix8zA54p12HLYIn4UNMjVEOjQl 0iTlReLDiXgE34TJj5Hz4uzpzvXC0066SUJHwotbfArDRFhCk9glwmCXK1ByvZLcaZ mL+RpvOLrlnT4HI5c8LzHfei9PievgMGvp/2K3JQERqntH6mzJVve7LmI+Rmr62esL oRv8B3iKwoeOzoy+oWPVFgjXVzkbUtIctaG5XeaVrqacMwCbl9zThTSnqR9h8/Bt8B IubkE3X8UqLtgj1rQZoM3R5bMjyyfpXBlqhYA3vSrN3XEn/UcKQyz3zcowbkknrSse Tb+kRn3WnFINg== Received: from [172.20.10.4] (unknown [89.113.152.201]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: a.rybakina@postgrespro.ru) by mail.postgrespro.ru (Postfix/587) with ESMTPSA id A82ED60A9A; Tue, 18 Feb 2025 10:24:03 +0300 (MSK) Content-Type: multipart/alternative; boundary="------------jj0oaY7ivcI0A4DPwtdSm4ab" Message-ID: <38e8cec8-bdef-4892-8e15-7f2207e5e157@postgrespro.ru> Date: Tue, 18 Feb 2025 10:24:02 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: Removing unneeded self joins To: Alexander Korotkov Cc: PostgreSQL Hackers , Tom Lane , Andrei Lepikhov , Robert Haas , Richard Guo , Alexander Lakhin , "Gregory Stark (as CFM)" , =?UTF-8?Q?Micha=C5=82_K=C5=82eczek?= , Dean Rasheed References: <64486b0b-0404-e39e-322d-0801154901f3@postgrespro.ru> <3622801.1715010885@sss.pgh.pa.us> <5934ce1e-be02-4640-904c-f7fec3962784@gmail.com> Content-Language: en-US From: Alena Rybakina In-Reply-To: X-KSMG-AntiPhishing: NotDetected, bases: 2025/02/18 05:40:00 X-KSMG-AntiSpam-Interceptor-Info: not scanned X-KSMG-AntiSpam-Status: not scanned, disabled by settings X-KSMG-AntiVirus: Kaspersky Secure Mail Gateway, version 2.1.0.7854, bases: 2025/02/17 19:12:00 #27315845 X-KSMG-AntiVirus-Status: NotDetected, skipped X-KSMG-LinksScanning: not scanned, disabled by settings X-KSMG-Message-Action: skipped X-KSMG-Rule-ID: 1 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk This is a multi-part message in MIME format. --------------jj0oaY7ivcI0A4DPwtdSm4ab Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 wrote: >> On Tue, Feb 11, 2025 at 5:31 PM Alena Rybakina >> 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 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 --------------jj0oaY7ivcI0A4DPwtdSm4ab Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit 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
--------------jj0oaY7ivcI0A4DPwtdSm4ab--