Re: postgres_fdw: wrong results with self join + enable_nestloop off

Hi,


Etsuro's patch is also showing the correct output for "set
enable_nestloop=off". Looks good to me for back branches due to backport
issues.

But below are a few observations for the same:-
1) I looked into the query plan for both "set enable_nestloop" on & off
case and observe that they are the same. That is, what we see with "set
enable_nestloop=on".
2) In back branches for "set enable_nestloop" on & off value, at least this
type of query execution won't make any difference. No comparison of plans
to be selected based on total cost of two plans old (Nested Loop with
Foreign Scans) & new (Only Foreign Scan) will be done, because we are
avoiding the call to "postgresGetForeignJoinPaths()" up front when we have
pseudo constants.


Regards,
Nishant.

On Tue, Jun 6, 2023 at 8:50 AM Richard Guo <guofenglinux@gmail.com> wrote:

>
> On Mon, Jun 5, 2023 at 9:19 PM Etsuro Fujita <etsuro.fujita@gmail.com>
> wrote:
>
>> If the patch is intended for HEAD only, I also think it goes in the
>> right direction.  But if it is intended for back branches as well, I
>> do not think so, because it would cause ABI breakage due to changes
>> made to the ForeignPath struct and the create_foreign_join_path() API.
>> (For the former, I think we could avoid doing so by adding the new
>> member at the end of the struct, not in the middle, though.)
>
>
> Thanks for pointing this out.  You're right.  The patch has backport
> issue because of the ABI breakage.  So it can only be applied on HEAD.
>
>
>> To avoid this issue, I am wondering if we should modify
>> add_paths_to_joinrel() in back branches so that it just disallows the
>> FDW to consider pushing down joins when the restrictlist has
>> pseudoconstant clauses.  Attached is a patch for that.
>
>
> I think we can do that in back branches.  But I'm a little concerned
> that we'd miss a better plan if FDW cannot push down joins in such
> cases.  I may be worrying over nothing though if it's not common that
> the restrictlist has pseudoconstant clauses.
>
> Thanks
> Richard
>