Remove an unnecessary check on semijoin_target_ok() on postgres_fdw.c

Started by Tender Wangover 1 year ago5 messages

tndrwang@gmail.com

over 1 year ago

1 attachment(s)

Hi,
When I debug FDW join pushdown codes, I found below codes in
semijoin_target_ok():
if (bms_is_member(var->varno, innerrel->relids) &&
!bms_is_member(var->varno, outerrel->relids))

As far as I know, if a var belongs to the innerrel of joinrel, it's not
possible that it
may belong to the outerrel. So if the bms_is_member(var->varno,
innerrel->relids)
returns TRUE, then !bms_is_member(var->varno, outerrel->relids) must be
TRUE.
If bms_is_member(var->varno, innerrel->relids) returns FALSE,
!bms_is_member(var->varno, outerrel->relids) will not execute due to short
circuit.

So I think we can remove the "!bms_is_member(var->varno, outerrel->relids)"
from if.
Any thoughts?

--
Thanks,
Tender Wang

Attachments:

0001-Remove-an-unnecessary-check-as-Var-can-only-belong-t.patchapplication/octet-stream; name=0001-Remove-an-unnecessary-check-as-Var-can-only-belong-t.patchDownload

From db668897442576429206a75c113f3d91db8de19b Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Wed, 9 Oct 2024 15:10:44 +0800
Subject: [PATCH] Remove an unnecessary check as Var can only belong to one
 sied of joinrel.

---
 contrib/postgres_fdw/postgres_fdw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index adc62576d1..6e1d2e636c 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -5771,12 +5771,11 @@ semijoin_target_ok(PlannerInfo *root, RelOptInfo *joinrel, RelOptInfo *outerrel,
 		if (!IsA(var, Var))
 			continue;
 
-		if (bms_is_member(var->varno, innerrel->relids) &&
-			!bms_is_member(var->varno, outerrel->relids))
+		if (bms_is_member(var->varno, innerrel->relids))
 		{
 			/*
 			 * The planner can create semi-join, which refers to inner rel
-			 * vars in its target list. However, we deparse semi-join as an
+			 * vars in its target list.  However, we deparse semi-join as an
 			 * exists() subquery, so can't handle references to inner rel in
 			 * the target list.
 			 */
-- 
2.25.1

Alexander Pyhalov

a.pyhalov@postgrespro.ru

about 1 year ago

In reply to: Tender Wang (#1)

Re: Remove an unnecessary check on semijoin_target_ok() on postgres_fdw.c

Tender Wang писал(а) 2024-10-09 10:26:

Hi,
When I debug FDW join pushdown codes, I found below codes in
semijoin_target_ok():
if (bms_is_member(var->varno, innerrel->relids) &&

!bms_is_member(var->varno, outerrel->relids))

As far as I know, if a var belongs to the innerrel of joinrel, it's
not possible that it
may belong to the outerrel. So if the bms_is_member(var->varno,
innerrel->relids)
returns TRUE, then !bms_is_member(var->varno, outerrel->relids) must
be TRUE.
If bms_is_member(var->varno, innerrel->relids) returns FALSE,
!bms_is_member(var->varno, outerrel->relids) will not execute due to
short circuit.

So I think we can remove the "!bms_is_member(var->varno,
outerrel->relids)" from if.
Any thoughts?

Hi.
Seems good to me.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Tender Wang

tndrwang@gmail.com

about 1 year ago

In reply to: Alexander Pyhalov (#2)

Re: Remove an unnecessary check on semijoin_target_ok() on postgres_fdw.c

Alexander Pyhalov <a.pyhalov@postgrespro.ru> 于2024年11月29日周五 00:02写道：

Tender Wang писал(а) 2024-10-09 10:26:

Hi,
When I debug FDW join pushdown codes, I found below codes in
semijoin_target_ok():
if (bms_is_member(var->varno, innerrel->relids) &&

!bms_is_member(var->varno, outerrel->relids))

As far as I know, if a var belongs to the innerrel of joinrel, it's
not possible that it
may belong to the outerrel. So if the bms_is_member(var->varno,
innerrel->relids)
returns TRUE, then !bms_is_member(var->varno, outerrel->relids) must
be TRUE.
If bms_is_member(var->varno, innerrel->relids) returns FALSE,
!bms_is_member(var->varno, outerrel->relids) will not execute due to
short circuit.

So I think we can remove the "!bms_is_member(var->varno,
outerrel->relids)" from if.
Any thoughts?

Hi.
Seems good to me.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Thanks for looking at that.

--
Thanks,
Tender Wang

Alexander Korotkov

aekorotkov@gmail.com

10 months ago

In reply to: Tender Wang (#3)

Re: Remove an unnecessary check on semijoin_target_ok() on postgres_fdw.c

On Fri, Nov 29, 2024 at 3:39 AM Tender Wang <tndrwang@gmail.com> wrote:

Alexander Pyhalov <a.pyhalov@postgrespro.ru> 于2024年11月29日周五 00:02写道：

Tender Wang писал(а) 2024-10-09 10:26:

Hi,
When I debug FDW join pushdown codes, I found below codes in
semijoin_target_ok():
if (bms_is_member(var->varno, innerrel->relids) &&

!bms_is_member(var->varno, outerrel->relids))

As far as I know, if a var belongs to the innerrel of joinrel, it's
not possible that it
may belong to the outerrel. So if the bms_is_member(var->varno,
innerrel->relids)
returns TRUE, then !bms_is_member(var->varno, outerrel->relids) must
be TRUE.
If bms_is_member(var->varno, innerrel->relids) returns FALSE,
!bms_is_member(var->varno, outerrel->relids) will not execute due to
short circuit.

So I think we can remove the "!bms_is_member(var->varno,
outerrel->relids)" from if.
Any thoughts?

Hi.
Seems good to me.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Thanks for looking at that.

Pushed. But I've decided to keep the redundant check as an assertion.

------
Regards,
Alexander Korotkov
Supabase

Tender Wang

tndrwang@gmail.com

10 months ago

In reply to: Alexander Korotkov (#4)

Re: Remove an unnecessary check on semijoin_target_ok() on postgres_fdw.c

Alexander Korotkov <aekorotkov@gmail.com> 于2025年3月25日周二 18:57写道：

On Fri, Nov 29, 2024 at 3:39 AM Tender Wang <tndrwang@gmail.com> wrote:

Alexander Pyhalov <a.pyhalov@postgrespro.ru> 于2024年11月29日周五 00:02写道：

Tender Wang писал(а) 2024-10-09 10:26:

Hi,
When I debug FDW join pushdown codes, I found below codes in
semijoin_target_ok():
if (bms_is_member(var->varno, innerrel->relids) &&

!bms_is_member(var->varno, outerrel->relids))

As far as I know, if a var belongs to the innerrel of joinrel, it's
not possible that it
may belong to the outerrel. So if the bms_is_member(var->varno,
innerrel->relids)
returns TRUE, then !bms_is_member(var->varno, outerrel->relids) must
be TRUE.
If bms_is_member(var->varno, innerrel->relids) returns FALSE,
!bms_is_member(var->varno, outerrel->relids) will not execute due to
short circuit.

So I think we can remove the "!bms_is_member(var->varno,
outerrel->relids)" from if.
Any thoughts?

Hi.
Seems good to me.
--
Best regards,
Alexander Pyhalov,
Postgres Professional

Thanks for looking at that.

Pushed. But I've decided to keep the redundant check as an assertion.

Thanks for pushing.

--
Thanks,
Tender Wang