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

I also agree that Richard's patch is better. As it fixes the issue at the
backend and does not treat pseudoconstant as local condition.

I have tested Richard's patch and observe that it is resolving the problem.
Patch looks good to me as well.

*I only had a minor comment on below change:-*





*-   gating_clauses = get_gating_quals(root, scan_clauses);+   if
(best_path->pathtype == T_ForeignScan && IS_JOIN_REL(rel))+
gating_clauses = get_gating_quals(root, ((ForeignPath *)
best_path)->joinrestrictinfo);+   else+       gating_clauses =
get_gating_quals(root, scan_clauses);*

>> Instead of using 'if' and creating a special case here can't we do
something in the above switch?


Regards,
Nishant.


P.S
I tried something quickly but I am seeing a crash:-
















*                case T_IndexOnlyScan:                        scan_clauses
= castNode(IndexPath, best_path)->indexinfo->indrestrictinfo;
          break;+               case T_ForeignScan:+
/*+                        * Note that for scans of foreign joins, we do
not have restriction clauses+                        * stored in
baserestrictinfo and we do not consider parameterization.+
       * Instead we need to check against joinrestrictinfo stored in
ForeignPath.+                        */+                       if
(IS_JOIN_REL(rel))+                               scan_clauses =
((ForeignPath *) best_path)->joinrestrictinfo;+                       else+
                              scan_clauses = rel->baserestrictinfo;+
                break;                default:
scan_clauses = rel->baserestrictinfo;                        break;*

On Fri, Jun 2, 2023 at 9:00 AM Suraj Kharage <suraj.kharage@enterprisedb.com>
wrote:

> +1 for fixing this in the backend code rather than FDW code.
>
> Thanks, Richard, for working on this. The patch looks good to me at
> a glance.
>
> On Tue, Apr 25, 2023 at 3:36 PM Richard Guo <guofenglinux@gmail.com>
> wrote:
>
>>
>> On Fri, Apr 14, 2023 at 8:51 PM Etsuro Fujita <etsuro.fujita@gmail.com>
>> wrote:
>>
>>> I think that the root cause for this issue would be in the
>>> create_scan_plan handling of pseudoconstant quals when creating a
>>> foreign-join (or custom-join) plan.
>>
>>
>> Yes exactly.  In create_scan_plan, we are supposed to extract all the
>> pseudoconstant clauses and use them as one-time quals in a gating Result
>> node.  Currently we check against rel->baserestrictinfo and ppi_clauses
>> for the pseudoconstant clauses.  But for scans of foreign joins, we do
>> not have any restriction clauses in these places and thus the gating
>> Result node as well as the pseudoconstant clauses would just be lost.
>>
>> I looked at Nishant's patch.  IIUC it treats the pseudoconstant clauses
>> as local conditions.  While it can fix the wrong results issue, I think
>> maybe it's better to still treat the pseudoconstant clauses as one-time
>> quals in a gating node.  So I wonder if we can store the restriction
>> clauses for foreign joins in ForeignPath, just as what we do for normal
>> JoinPath, and then check against them for pseudoconstant clauses in
>> create_scan_plan, something like attached.
>>
>> BTW, while going through the codes I noticed one place in
>> add_foreign_final_paths that uses NULL for List *.  I changed it to NIL.
>>
>> Thanks
>> Richard
>>
>
>
> --
> --
>
> Thanks & Regards,
> Suraj kharage,
>
>
>
> edbpostgres.com
>