GetExistingLocalJoinPath() vs. the docs

Started by Stephen Frostalmost 10 years ago5 messages
#1Stephen Frost
sfrost@snowman.net

Greetings,

While getting back into the thread Re: Optimization for updating foreign
tables in Postgres FDW, I noticed some issues with the docs and
GetExistingLocalJoinPath():

GetExistingLocalJoinPath() exists in src/backend/foreign/foreign.c, but
the docs include discussion of it under 54.2 - Foreign Data Wrapper
Callback Routines. Shouldn't this be under 54.3. Foreign Data Wrapper
Helper Functions? Also, the prototype is incorrect in the docs (it
doesn't return a void) and the paragraph about what it's for could do
with some wordsmithing.

A link from 54.2 to 54.3 which mentions it would be fine, of course.

Thanks!

Stephen

#2Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Stephen Frost (#1)
1 attachment(s)
Re: GetExistingLocalJoinPath() vs. the docs

On Mon, Feb 15, 2016 at 9:11 PM, Stephen Frost <sfrost@snowman.net> wrote:

Greetings,

While getting back into the thread Re: Optimization for updating foreign
tables in Postgres FDW, I noticed some issues with the docs and
GetExistingLocalJoinPath():

GetExistingLocalJoinPath() exists in src/backend/foreign/foreign.c, but
the docs include discussion of it under 54.2 - Foreign Data Wrapper
Callback Routines. Shouldn't this be under 54.3. Foreign Data Wrapper
Helper Functions?

Yes

Also, the prototype is incorrect in the docs (it
doesn't return a void)

Thanks for pointing that out.

and the paragraph about what it's for could do
with some wordsmithing.

Any specific suggestions?

A link from 54.2 to 54.3 which mentions it would be fine, of course.

Ok

PFA patch fixing those things.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Attachments:

geljp_doc.patchapplication/x-download; name=geljp_doc.patchDownload
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index ffbd1e3..d018edb 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -334,34 +334,20 @@ GetForeignJoinPaths (PlannerInfo *root,
      remote join cannot be found from the system catalogs, the FDW must
      fill <structfield>fdw_scan_tlist</> with an appropriate list
      of <structfield>TargetEntry</> nodes, representing the set of columns
      it will supply at run time in the tuples it returns.
     </para>
 
     <para>
      See <xref linkend="fdw-planning"> for additional information.
     </para>
 
-    <para>
-<programlisting>
-void
-GetExistingLocalJoinPath(RelOptInfo *joinrel)
-</programlisting>
-     The function returns copy of a local join path, which can be converted
-     into an alternative local join plan, which may be useful when
-     implementing a <literal>RecheckForeignScan</> method.  The function
-     searches for an unparameterized path in the <literal>pathlist</> of given
-     <literal>joinrel</>. If it does not find such a path, it returns NULL, in
-     which case a foreign data wrapper may build the local path by itself or
-     may choose not to create access paths for that join.
-    </para>
-
    </sect2>
 
    <sect2 id="fdw-callbacks-update">
     <title>FDW Routines For Updating Foreign Tables</title>
 
     <para>
      If an FDW supports writable foreign tables, it should provide
      some or all of the following callback functions depending on
      the needs and capabilities of the FDW:
     </para>
@@ -803,21 +789,21 @@ RecheckForeignScan (ForeignScanState *node, TupleTableSlot *slot);
     <para>
      To implement join pushdown, a foreign data wrapper will typically
      construct an alternative local join plan which is used only for
      rechecks; this will become the outer subplan of the
      <literal>ForeignScan</>.  When a recheck is required, this subplan
      can be executed and the resulting tuple can be stored in the slot.
      This plan need not be efficient since no base table will return more
      than one row; for example, it may implement all joins as nested loops.
      <literal>GetExistingLocalJoinPath</> may be used to search existing paths
      for a suitable local join path, which can be used as the alternative
-     local join plan.
+     local join plan. See <xref linkend="fdw-helpers"> for details.
     </para>
    </sect2>
 
    <sect2 id="fdw-callbacks-explain">
     <title>FDW Routines for <command>EXPLAIN</></title>
 
     <para>
 <programlisting>
 void
 ExplainForeignScan (ForeignScanState *node,
@@ -1131,20 +1117,34 @@ GetForeignDataWrapperByName(const char *name, bool missing_ok);
 <programlisting>
 ForeignServer *
 GetForeignServerByName(const char *name, bool missing_ok);
 </programlisting>
 
      This function returns a <structname>ForeignServer</structname> object
      for the foreign server with the given name.  If the server is not found,
      return NULL if missing_ok is true, otherwise raise an error.
     </para>
 
+    <para>
+<programlisting>
+Path *
+GetExistingLocalJoinPath(RelOptInfo *joinrel)
+</programlisting>
+     The function returns copy of a local join path, which can be converted
+     into an alternative local join plan, which may be useful when
+     implementing a <literal>RecheckForeignScan</> method.  The function
+     searches for an unparameterized path in the <literal>pathlist</> of given
+     <literal>joinrel</>. If it does not find such a path, it returns NULL, in
+     which case a foreign data wrapper may build the local path by itself or
+     may choose not to create access paths for that join.
+    </para>
+
    </sect1>
 
    <sect1 id="fdw-planning">
     <title>Foreign Data Wrapper Query Planning</title>
 
     <para>
      The FDW callback functions <function>GetForeignRelSize</>,
      <function>GetForeignPaths</>, <function>GetForeignPlan</>,
      <function>PlanForeignModify</>, and <function>GetForeignJoinPaths</>
      must fit into the workings of the <productname>PostgreSQL</> planner.
#3Robert Haas
robertmhaas@gmail.com
In reply to: Ashutosh Bapat (#2)
1 attachment(s)
Re: GetExistingLocalJoinPath() vs. the docs

On Tue, Feb 16, 2016 at 7:53 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

PFA patch fixing those things.

I think that you need to take a little broader look at this section.
At the top, it says "To use any of these functions, you need to
include the header file foreign/foreign.h in your source file", but
this function is defined in foreign/fdwapi.h. It's not clear to me
whether we should consider moving the prototype, or just document that
this function is someplace else. The other functions prototyped in
fdwapi.h aren't documented at all, except for
IsImportableForeignTable, which is mentioned in passing.

Further down, the section says "Some object types have name-based
lookup functions in addition to the OID-based ones:" and you propose
to put the documentation for this function after that. But this
comment doesn't actually describe this particular function.

Actually, this function just doesn't seem to fit into this section at
all. It's really quite different from the others listed there. How
about something like the attached instead?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Attachments:

gejlp-rmh-doc.patchapplication/x-download; name=gejlp-rmh-doc.patchDownload
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 9eec3c8..9ad4e1c 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -340,21 +340,6 @@ GetForeignJoinPaths (PlannerInfo *root,
     <para>
      See <xref linkend="fdw-planning"> for additional information.
     </para>
-
-    <para>
-<programlisting>
-void
-GetExistingLocalJoinPath(RelOptInfo *joinrel)
-</programlisting>
-     The function returns copy of a local join path, which can be converted
-     into an alternative local join plan, which may be useful when
-     implementing a <literal>RecheckForeignScan</> method.  The function
-     searches for an unparameterized path in the <literal>pathlist</> of given
-     <literal>joinrel</>. If it does not find such a path, it returns NULL, in
-     which case a foreign data wrapper may build the local path by itself or
-     may choose not to create access paths for that join.
-    </para>
-
    </sect2>
 
    <sect2 id="fdw-callbacks-update">
@@ -808,9 +793,13 @@ RecheckForeignScan (ForeignScanState *node, TupleTableSlot *slot);
      can be executed and the resulting tuple can be stored in the slot.
      This plan need not be efficient since no base table will return more
      than one row; for example, it may implement all joins as nested loops.
-     <literal>GetExistingLocalJoinPath</> may be used to search existing paths
-     for a suitable local join path, which can be used as the alternative
-     local join plan.
+     The function <literal>GetExistingLocalJoinPath</> may be used to search
+     existing paths for a suitable local join path, which can be used as the
+     alternative local join plan.  <literal>GetExistingLocalJoinPath</>
+     searches for an unparameterized path in the path list of the specified
+     join relation.  (If it does not find such a path, it returns NULL, in
+     which case a foreign data wrapper may build the local path by itself or
+     may choose not to create access paths for that join.)
     </para>
    </sect2>
 
#4Ashutosh Bapat
ashutosh.bapat@enterprisedb.com
In reply to: Robert Haas (#3)
Re: GetExistingLocalJoinPath() vs. the docs

I think that you need to take a little broader look at this section.

At the top, it says "To use any of these functions, you need to
include the header file foreign/foreign.h in your source file", but
this function is defined in foreign/fdwapi.h. It's not clear to me
whether we should consider moving the prototype, or just document that
this function is someplace else. The other functions prototyped in
fdwapi.h aren't documented at all, except for
IsImportableForeignTable, which is mentioned in passing.

Further down, the section says "Some object types have name-based
lookup functions in addition to the OID-based ones:" and you propose
to put the documentation for this function after that. But this
comment doesn't actually describe this particular function.

Actually, this function just doesn't seem to fit into this section at
all. It's really quite different from the others listed there. How
about something like the attached instead?

Right. Mentioning the function in the description of relevant function
looks better and avoids some duplication.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

#5Robert Haas
robertmhaas@gmail.com
In reply to: Ashutosh Bapat (#4)
Re: GetExistingLocalJoinPath() vs. the docs

On Wed, Mar 2, 2016 at 1:12 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

I think that you need to take a little broader look at this section.
At the top, it says "To use any of these functions, you need to
include the header file foreign/foreign.h in your source file", but
this function is defined in foreign/fdwapi.h. It's not clear to me
whether we should consider moving the prototype, or just document that
this function is someplace else. The other functions prototyped in
fdwapi.h aren't documented at all, except for
IsImportableForeignTable, which is mentioned in passing.

Further down, the section says "Some object types have name-based
lookup functions in addition to the OID-based ones:" and you propose
to put the documentation for this function after that. But this
comment doesn't actually describe this particular function.

Actually, this function just doesn't seem to fit into this section at
all. It's really quite different from the others listed there. How
about something like the attached instead?

Right. Mentioning the function in the description of relevant function looks
better and avoids some duplication.

Cool, committed that way.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers