MIME-Version: 1.0
Date: Tue, 05 Dec 2023 13:29:47 +0300
From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
 "Fujii.Yuki@df.MitsubishiElectric.co.jp"
 <Fujii.Yuki@df.mitsubishielectric.co.jp>, Ashutosh Bapat
 <ashutosh.bapat.oss@gmail.com>, Pgsql Hackers
 <pgsql-hackers@postgresql.org>, Ian Lawrence Barwick <barwick@gmail.com>
Subject: Re: Add semi-join pushdown to postgres_fdw
In-Reply-To: <CAPpHfdsbqy93Og9_0UrtNmCxE7LRwLWLTrmeHkOfeWtvg5tSAA@mail.gmail.com>
References: <c9e2a757cf3ac2333714eaf83a9cc184@postgrespro.ru>
 <CAExHW5u+wy7YAp61o-QJ=p8CMjZCZbcin=_xh7bYjeyJV1a1pg@mail.gmail.com>
 <10b3408a0880462d2ec6e3a92a46800a@postgrespro.ru>
 <CAB8KJ=hXB8=JfFM5e+UTkEDTNU0gA8Xr-nToMNGahZc7ktD-gA@mail.gmail.com>
 <81d8befc64cfd7ccc4a3f3e5c6862f20@postgrespro.ru>
 <OS3PR01MB6660A7F36A2D37194DB601F195169@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <816fa8b1bc2da09a87484d1ef239a332@postgrespro.ru>
 <4d6b5ec7-1583-f253-c6d2-810602b4b2aa@enterprisedb.com>
 <f929c315c6f3e9114ffc53e29fe83689@postgrespro.ru>
 <CAPpHfdtLSp3YnmbTMpUZvBa=kJ3qLosUQVjNfW3__=oWUP8dkg@mail.gmail.com>
 <3dd9320e01232eb931b747ed75a26cfb@postgrespro.ru>
 <CAPpHfdtYndP0+P4YjJrNAgX6X6xVL6z4n0L_u3P=se+Xt0VksA@mail.gmail.com>
 <f76e5ec21f827bcc4ceca4b398ba7289@postgrespro.ru>
 <CAPpHfdsbqy93Og9_0UrtNmCxE7LRwLWLTrmeHkOfeWtvg5tSAA@mail.gmail.com>
User-Agent: Roundcube Webmail/1.6.5
Message-ID: <b333e7556b6c75400ec3176fcf3a4665@postgrespro.ru>
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/b333e7556b6c75400ec3176fcf3a4665%40postgrespro.ru>
Precedence: bulk

Alexander Korotkov писал(а) 2023-12-03 23:52:
> Hi, Alexander!
> 
> On Mon, Nov 27, 2023 at 5:11 PM Alexander Pyhalov
> <a.pyhalov@postgrespro.ru> wrote:
>> Alexander Korotkov писал(а) 2023-11-27 03:49:
>> 
>> > Thank you for the revision.
>> >
>> > I've revised the patch myself.  I've replaced StringInfo with
>> > additional conds into a list of strings as I proposed before.  I think
>> > the code became much clearer.  Also, it gets rid of some unnecessary
>> > allocations.
>> >
>> > I think the code itself is not in bad shape.  But patch lacks some
>> > high-level description of semi-joins processing as well as comments on
>> > each manipulation with additional conds.  Could you please add this?
>> >
>> 
>> Hi. The updated patch looks better. It seems I've failed to fix logic 
>> in
>> deparseFromExprForRel() when tried to convert StringInfos to Lists.
>> 
>> I've added some comments. The most complete description of how 
>> SEMI-JOIN
>> is processed, is located in deparseFromExprForRel(). Unfortunately,
>> there seems to be no single place, describing current JOIN deparsing
>> logic.
> 
> Looks good to me. I've made some grammar and formatting adjustments.
> Also, I've written the commit message.
> 
> Now, I think this looks good.  I'm going to push this if no objections.
> 
> ------
> Regards,
> Alexander Korotkov

Hi. No objections from my side.

Perhaps, some rephrasing is needed in comment in semijoin_target_ok():

"The planner can create semi-joins, which refer to inner rel
vars in its target list."

Perhaps, change "semi-joins, which refer" to "a semi-join, which refers 
...",
as later we speak about "its" target list.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional