Re: [v9.3] writable foreign tables - Mailing list pgsql-hackers

2013/2/5 Daniel Farina <daniel@heroku.com>:
> On Fri, Feb 1, 2013 at 2:22 AM, Daniel Farina <daniel@heroku.com> wrote:
>> On Tue, Jan 29, 2013 at 1:19 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> I noticed the v10 patch cannot be applied on the latest master branch
>>> cleanly. The attached ones were rebased.
>>
>> Anyway, I'm looking at the first patch, which applies neatly. Thanks.
>
> Hello,
>
> My review is incomplete, but to the benefit of pipelining I wanted to
> ask a couple of things and submit some changes for your consideration
> while continuing to look at this.
>
> So far, I've been trying to understand in very close detail how your
> changes affect non-FDW related paths first, before delving into the
> actual writable FDW functionality.  There's not that much in this
> category, but there's one thing that gave me pause: the fact that the
> target list (by convention, tlist) has to be pushed down from
> planmain.c/query_planner all the way to a
> fdwroutine->GetForeignRelWidth callsite in plancat.c/get_relation_info
> (so even in the end, it is just pushed down -- never inspected or
> modified).  In relative terms, it's a significant widening of
> currently fairly short parameter lists in these procedures:
>
> add_base_rels_to_query(PlannerInfo *root, List *tlist, Node *jtnode)
> build_simple_rel(PlannerInfo *root, int relid, List *tlist, RelOptKind
> reloptkind)
> get_relation_info(PlannerInfo *root, Oid relationObjectId, bool
> inhparent, List *tlist, RelOptInfo *rel)
>
> In the case of all the other parameters except tlist, each is more
> intimately related with the inner workings and mechanisms of the
> procedure.  I wonder if there's a nice way that can train the reader's
> eye that the tlist is simply pushed down rather than actively managed
> at each level.  However, I can't immediately produce a way to both
> achieve that that doesn't seem overwrought.  Perhaps a comment will
> suffice.
>
Thanks for your checks.

add_base_rels_to_query() originally has two arguments; PlannerInfo
and Node, but these are wide-spreading in use. I don't think it is a good
idea to add a special field to reduce number of arguments only.
Instead, I tried to add a short comment on top of add_base_rels_to_query()
as follows.

 * XXX - Also note that tlist needs to be pushed down into deeper level,
 * for construction of RelOptInfo relevant to foreign-tables with pseudo-
 * columns.
 */

I'm not 100% sure whether it is a good comment here, because internal
will be revised patch-by-patch. So, if future version also modifies argument
list here, this comment may talks a lie.

> Related to this, I found that make_modifytable grew a dependency on
> PlannerInfo *root, and it seemed like a bunch of the arguments are
> functionally related to that, so the attached patch that should be
> able to be applied to yours tries to utilize that as often as
> possible.  The weirdest thing in there is how make_modifytable has
> been taught to call SS_assign_special_param, which has a side effect
> on the PlannerInfo, but there exist exactly zero cases where one
> *doesn't* want to do that prior to the (exactly two) call sites to
> make_modifytable, so I pushed it in.  The second weirdest thing is
> pushing in the handling of rowMarks (e.g. SELECT FOR UPDATE et al)
>
Good suggestion. I added the PlannerInfo *root with spinal-reflexes.
Indeed, I'd like to agree with your optimization.

The attached patch adds Daniel's reworks on make_modifytable
invocation, and add a short comment on add_base_rels_to_query().
Rest of portion has not been changed from the previous version.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>