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(dot)kharage(at)enterprisedb(dot)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(at)gmail(dot)com>
> wrote:
>
>>
>> On Fri, Apr 14, 2023 at 8:51 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)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
>