Re: Add semi-join pushdown to postgres_fdw

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