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

Hi Richard,

On Sun, Jun 25, 2023 at 3:05 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
> On Wed, Jun 14, 2023 at 2:49 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
>> On Mon, Jun 5, 2023 at 10:19 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)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