Re: [v9.3] writable foreign tables
От | Kohei KaiGai |
---|---|
Тема | Re: [v9.3] writable foreign tables |
Дата | |
Msg-id | CADyhKSX=-jabz5GbqxSj++uP5pYBD+vVje9WpoU0iZa3G_s-Ug@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [v9.3] writable foreign tables (Daniel Farina <daniel@heroku.com>) |
Ответы |
Re: [v9.3] writable foreign tables
(Craig Ringer <craig@2ndquadrant.com>)
Re: [v9.3] writable foreign tables (Tom Lane <tgl@sss.pgh.pa.us>) |
Список | 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>
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Dimitri FontaineДата:
Сообщение: Re: proposal: ANSI SQL 2011 syntax for named parameters