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

Hi Richard,

On Mon, Jul 31, 2023 at 5:52 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Fri, Jul 28, 2023 at 4:56 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>> here is a rebased version of the second patch, in which I modified the
>> ForeignPath and CustomPath cases in reparameterize_path_by_child() to
>> reflect the new members fdw_restrictinfo and custom_restrictinfo, for
>> safety, and tweaked a comment a bit.

> Hmm, it seems that ForeignPath for a foreign join does not support
> parameterized paths for now, as in postgresGetForeignJoinPaths() we have
> this check:
>
>   /*
>    * This code does not work for joins with lateral references, since those
>    * must have parameterized paths, which we don't generate yet.
>    */
>   if (!bms_is_empty(joinrel->lateral_relids))
>       return;
>
> And in create_foreign_join_path() we just set the path.param_info to
> NULL.
>
>   pathnode->path.param_info = NULL;   /* XXX see above */
>
> So I doubt that it's necessary to adjust fdw_restrictinfo in
> reparameterize_path_by_child, because it seems to me that
> fdw_restrictinfo must be empty there.  Maybe we can add an Assert there
> as below:
>
> -        ADJUST_CHILD_ATTRS(fpath->fdw_restrictinfo);
> +
> +        /*
> +         * Parameterized foreign joins are not supported.  So this
> +         * ForeignPath cannot be a foreign join and fdw_restrictinfo
> +         * must be empty.
> +         */
> +        Assert(fpath->fdw_restrictinfo == NIL);
>
> That being said, it's also no harm to handle fdw_restrictinfo in
> reparameterize_path_by_child as the patch does.  So I'm OK we do that
> for safety.

Ok, but maybe my explanation was not good, so let me explain.  The
reason why I modified the code as such is to make the handling of
fdw_restrictinfo consistent with that of fdw_outerpath: we have the
code to reparameterize fdw_outerpath, which should be NULL though, as
we do not currently support parameterized foreign joins.

I modified the code a bit further to use an if-test to avoid a useless
function call, and added/tweaked comments and docs further.  Attached
is a new version of the patch.  I am planning to commit this, if there
are no objections.

Thanks!

Best regards,
Etsuro Fujita