[prev in list] [next in list] [prev in thread] [next in thread] 

List:       postgresql-hackers
Subject:    Re: postgres_fdw: wrong results with self join + enable_nestloop off
From:       Richard Guo <guofenglinux () gmail ! com>
Date:       2023-07-31 8:52:16
Message-ID: CAMbWs4-3iptx7oJDFF1ACSU-1SjcahHxfs62kve4N=KCnazVYQ () mail ! gmail ! com
[Download RAW message or body]

On Fri, Jul 28, 2023 at 4:56 PM Etsuro Fujita <etsuro.fujita@gmail.com>
wrote:

> Cool!  I pushed the first patch after polishing it a little bit, so
> 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.

Thanks
Richard

[Attachment #3 (text/html)]

<div dir="ltr"><div dir="ltr"><br></div><div class="gmail_quote"><div dir="ltr" \
class="gmail_attr">On Fri, Jul 28, 2023 at 4:56 PM Etsuro Fujita &lt;<a \
href="mailto:etsuro.fujita@gmail.com">etsuro.fujita@gmail.com</a>&gt; \
wrote:</div><blockquote class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Cool!   I pushed the \
first patch after polishing it a little bit, so<br> here is a rebased version of the \
second patch, in which I modified the<br> ForeignPath and CustomPath cases in \
reparameterize_path_by_child() to<br> reflect the new members fdw_restrictinfo and \
custom_restrictinfo, for<br> safety, and tweaked a comment a \
bit.</blockquote><div><br></div><div>Hmm, it seems that ForeignPath for a foreign \
join does not support<br>parameterized paths for now, as in \
postgresGetForeignJoinPaths() we have<br>this check:<br><br>   /*<br>     * This code \
does not work for joins with lateral references, since those<br>     * must have \
parameterized paths, which we don&#39;t generate yet.<br>     */<br>   if \
(!bms_is_empty(joinrel-&gt;lateral_relids))<br>         return;<br><br>And in \
create_foreign_join_path() we just set the path.param_info to<br>NULL.<br><br>   \
pathnode-&gt;path.param_info = NULL;    /* XXX see above */<br><br>So I doubt that \
it&#39;s necessary to adjust fdw_restrictinfo in<br>reparameterize_path_by_child, \
because it seems to me that<br>fdw_restrictinfo must be empty there.   Maybe we can \
add an Assert there<br>as below:<br><br>-            \
ADJUST_CHILD_ATTRS(fpath-&gt;fdw_restrictinfo);<br>+<br>+            /*<br>+          \
* Parameterized foreign joins are not supported.   So this<br>+             * \
ForeignPath cannot be a foreign join and fdw_restrictinfo<br>+             * must be \
empty.<br>+             */<br>+            Assert(fpath-&gt;fdw_restrictinfo == \
NIL);<br><br>That being said, it&#39;s also no harm to handle fdw_restrictinfo \
in<br>reparameterize_path_by_child as the patch does.   So I&#39;m OK we do \
that<br>for safety.<br><br>Thanks<br>Richard<br></div></div></div>



[prev in list] [next in list] [prev in thread] [next in thread]