Hi Richard, On Sun, Jun 25, 2023 at 3:05 PM Richard Guo <guofengli...@gmail.com> wrote: > On Wed, Jun 14, 2023 at 2:49 PM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote: >> On Mon, Jun 5, 2023 at 10:19 PM Etsuro Fujita <etsuro.fuj...@gmail.com> >> wrote: >> > 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 that custom scans have the same issue, so I modified the patch >> further so that it also disallows custom-scan providers to consider >> join pushdown in add_paths_to_joinrel() if necessary. Attached is a
> Good point. The v2 patch looks good to me for back branches. Cool! Thanks for looking! > I'm wondering what the plan is for HEAD. Should we also disallow > foreign/custom join pushdown in the case that there is any > pseudoconstant restriction clause, or instead still allow join pushdown > in that case? If it is the latter, I think we can do something like my > patch upthread does. But that patch needs to be revised to consider > custom scans, maybe by storing the restriction clauses also in > CustomPath? I think we should choose the latter, so I modified your patch as mentioned, after re-creating it on top of my patch. Attached is a new version (0002-Allow-join-pushdown-even-if-pseudoconstant-quals-v2.patch). I am attaching my patch as well (0001-Disable-join-pushdown-if-pseudoconstant-quals-v2.patch). Other changes made to your patch: * I renamed the new member of the ForeignPath struct to fdw_restrictinfo. (And I named that of the CustomPath struct custom_restrictinfo.) * In your patch, only for create_foreign_join_path(), the API was modified so that the caller provides the new member of ForeignPath, but I modified that for create_foreignscan_path()/create_foreign_upper_path() as well, for consistency. * In this bit I changed the last argument to NIL, which would be nitpicking, though. @@ -1038,7 +1038,7 @@ postgresGetForeignPaths(PlannerInfo *root, add_path(baserel, (Path *) path); /* Add paths with pathkeys */ - add_paths_with_pathkeys_for_rel(root, baserel, NULL); + add_paths_with_pathkeys_for_rel(root, baserel, NULL, NULL); * I dropped this test case, because it would not be stable if the system clock was too slow. +-- bug due to sloppy handling of pseudoconstant clauses for foreign joins +EXPLAIN (VERBOSE, COSTS OFF) + SELECT * FROM ft2 a, ft2 b + WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp; +SELECT * FROM ft2 a, ft2 b +WHERE b.c1 = a.c1 AND now() < '25-April-2023'::timestamp; That is it. Sorry for the long long delay. Best regards, Etsuro Fujita
0001-Disable-join-pushdown-if-pseudoconstant-quals-v2.patch
Description: Binary data
0002-Allow-join-pushdown-even-if-pseudoconstant-quals-v2.patch
Description: Binary data
