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.khar...@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 <guofengli...@gmail.com> > wrote: > >> >> On Fri, Apr 14, 2023 at 8:51 PM Etsuro Fujita <etsuro.fuj...@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 >