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(at)gmail(dot)com> wrote:
> On Fri, Jul 28, 2023 at 4:56 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)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