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

On Fri, Jul 21, 2023 at 8:51 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com>
wrote:

> 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.)

That's much better, and more consistent with other members in
ForeignPath/CustomPath. Thanks!

> * 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.

LGTM.

> * 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);

Good catch! This was my oversight.

> * I dropped this test case, because it would not be stable if the
> system clock was too slow.

Agreed. And the test case from 0001 should be sufficient.

So the two patches both look good to me now.

Thanks
Richard