Re: postgres_fdw: wrong results with self join + enable_nestloop off - Mailing list pgsql-hackers
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);
+ 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;
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----
pgsql-hackers by date: