On Thu, Mar 12, 2015 at 8:09 PM, Thom Brown <thom@linux.com> wrote:
> On 12 March 2015 at 21:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> I took a look at this patch today and noticed that it incorporates not
>>> only documentation for the new functionality it adds, but also for the
>>> custom-scan functionality whose documentation I previously excluded
>>> from commit on the grounds that it needed more work, especially to
>>> improve the English. That decision was not popular at the time, and I
>>> think we need to remedy it before going further with this. I had
>>> hoped that someone else would care about this work enough to help with
>>> the documentation, but it seems not, so today I went through the
>>> documentation in this patch, excluded all of the stuff specific to
>>> custom joins, and heavily edited the rest. The result is attached.
>>
>> Looks good; I noticed one trivial typo --- please s/returns/return/ under
>> ExecCustomScan. Also, perhaps instead of just "set ps_ResultTupleSlot"
>> that should say "fill ps_ResultTupleSlot with the next tuple in the
>> current scan direction".
>
> Also:
>
> s/initalization/initialization/
Thanks to both of you for the review. I have committed it with those
improvements. Please let me know if you spot anything else.
Another bit of this that I think we could commit without fretting
about it too much is the code adding set_join_pathlist_hook. This is
- I think - analogous to set_rel_pathlist_hook, and like that hook,
could be used for other purposes than custom plan generation - e.g. to
delete paths we do not want to use. I've extracted this portion of
the patch and adjusted the comments; if there are no objections, I
will commit this bit also.
Kaigai, note that your patch puts this hook and the call to
GetForeignJoinPaths() in the wrong order. As in the baserel case, the
hook should get the last word, after any FDW-specific handlers have
been called, so that it can delete or modify paths as well as add
them.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company