Re: Custom/Foreign-Join-APIs (Re: [v9.5] Custom Plan API)

On Sun, Apr 26, 2015 at 10:00 PM, Kouhei Kaigai <kaigai(at)ak(dot)jp(dot)nec(dot)com> wrote:
> The attached patch v13 is revised one according to the suggestion
> by Robert.

Thanks.

The last hunk in foreign.c is a useless whitespace change.

+ /* actually, not shift members */

Change to: "shift of 0 is the same as copying"

But actually, do we really need all of this? I think you could reduce
the size of this function to three lines of code if you just did this:

x = -1;
while ((x = bms_next_member(inputset, x)) >= 0)
outputset = bms_add_member(inputset, x + shift);

It might be very slightly slower, but I think it would be worth it to
reduce the amount of code needed.

+ * 5. Consider paths added by FDW, in case when both of outer and
+ * inner relations are managed by the same driver.

Change to: "If both inner and outer relations are managed by the same
FDW, give it a chance to push down joins."

+ * 6. At the last, consider paths added by extension, in addition to the
+ * built-in paths.

Change to: "Finally, give extensions a chance to manipulate the path list."

+ * Fetch relation-id, if this foreign-scan node actuall scans on
+ * a particular real relation. Elsewhere, InvalidOid shall be
+ * informed to the FDW driver.

Change to: "If we're scanning a base relation, look up the OID. (We
can skip this if scanning a join relation.)"

+ * Sanity check. Pseudo scan tuple-descriptor shall be constructed
+ * based on the fdw_ps_tlist, excluding resjunk=true, so we need to
+ * ensure all valid TLEs have to locate prior to junk ones.

Is the goal here to make attribute numbers match up? If so, between
where and where? If not, please explain further.

+ if (splan->scan.scanrelid == 0)
+ {
...
+ }
splan->scan.scanrelid += rtoffset;

Does this need an "else"? It seems surprising that you would offset
scanrelid even if it's starting out as zero.

(Note that there are two instances of this pattern.)

+ * 'found' : indicates whether RelOptInfo is actually constructed.
+ * true, if it was already built and on the cache.

Leftover hunk. Revert this.

+typedef void (*GetForeignJoinPaths_function ) (PlannerInfo *root,

Whitespace is wrong, still.

+ * An optional fdw_ps_tlist is used to map a reference to an attribute of
+ * underlying relation(s) on a pair of INDEX_VAR and alternative varattno.

on -> onto

+ * It looks like a scan on pseudo relation that is usually result of
+ * relations join on remote data source, and FDW driver is responsible to
+ * set expected target list for this.

Change to: "When fdw_ps_tlist is used, this represents a remote join,
and the FDW driver is responsible for setting this field to an
appropriate value."

If FDW returns records as foreign-
+ * table definition, just put NIL here.

I think this is just referring to the non-join case; if so, just drop
it. Otherwise, I'm confused and need a further explanation.

+ * Note that since Plan trees can be copied, custom scan providers *must*

Extra space before "Note"

+ Bitmapset *custom_relids; /* set of relid (index of range-tables)
+ *
represented by this node */

Maybe "RTIs this node generates"?

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