Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables
От | Robert Haas |
---|---|
Тема | Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables |
Дата | |
Msg-id | CA+TgmobQK80vtXjAsPZWWXd7c8u13G86gmuLupN+uUJjA+i4nA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables (Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>) |
Ответы |
Re: [HACKERS] Partition-wise join for join between (declaratively)partitioned tables
(Ashutosh Bapat <ashutosh.bapat@enterprisedb.com>)
|
Список | pgsql-hackers |
On Fri, Jul 14, 2017 at 3:02 AM, Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> wrote: > Here's revised patch set with only 0004 revised. That patch deals with > creating multi-level inheritance hierarchy from multi-level partition > hierarchy. The original logic of recursively calling > inheritance_planner()'s guts over the inheritance hierarchy required > that for every such recursion we flatten many lists created by that > code. Recursion also meant that root->append_rel_list is traversed as > many times as the number of partitioned partitions in the hierarchy. > Instead the revised version keep the iterative shape of > inheritance_planner() intact, thus naturally creating flat lists, > iterates over root->append_rel_list only once and is still easy to > read and maintain. 0001-0003 look basically OK to me, modulo some cosmetic stuff. Regarding 0004: + if (brel->reloptkind != RELOPT_BASEREL && + brte->relkind != RELKIND_PARTITIONED_TABLE) I spent a lot of time staring at this code before I figured out what was going on here. We're iterating over simple_rel_array, so the reloptkind must be RELOPT_OTHER_MEMBER_REL if it isn't RELOPT_BASEREL. But does that guarantee that rtekind is RTE_RELATION such that brte->relkind will be initialized to a value? I'm not 100% sure. I think it would be clearer to write this test like this: Assert(IS_SIMPLE_REL(brel)); if (brel->reloptkind == RELOPT_OTHER_MEMBER_REL && (brte->rtekind != RELOPT_BASEREL || brte->relkind != RELKIND_PARTITIONED_TABLE)) continue; Note that the way you wrote the comment is says if it *is* another REL, not if it's *not* a baserel; it's good if those kinds of little details match between the code and the comments. It is not clear to me what the motivation is for the API changes in expanded_inherited_rtentry. They don't appear to be necessary. If they are necessary, you need to do a more thorough job updating the comments. This one, in particular: * If so, add entries for all the child tables to the query's * rangetable, and build AppendRelInfo nodes for allthe child tables * and add them to root->append_rel_list. If not, clear the entry's And the comments could maybe say something like "We return the list of appinfos rather than directly appending it to append_rel_list because $REASON." - * is a partitioned table. + * RTE simply duplicates the parent *partitioned* table. */ - if (childrte->relkind != RELKIND_PARTITIONED_TABLE) + if (childrte->relkind != RELKIND_PARTITIONED_TABLE || childrte->inh) This is another case where it's hard to understand the test from the comments. + * In case of multi-level inheritance hierarchy, for every child we require + * PlannerInfo of its immediate parent. Hence we save those in a an array Say why. Also, need to fix "a an". I'm a little bit surprised that this patch doesn't make any changes to allpaths.c or relnode.c. It looks to me like we'll generate paths for the new RTEs that are being added. Are we going to end up with multiple levels of Append nodes, then? Does the consider the way consider_parallel is propagated up and down in set_append_rel_size() and set_append_rel_pathlist() really work with multiple levels? Maybe this is all fine; I haven't tried to verify it in depth. Overall I think this is a reasonable direction to go but I'm worried that there may be bugs lurking -- other code that needs adjusting that hasn't been found, really. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Tatsuo IshiiДата:
Сообщение: Re: [HACKERS] Missing comment for max_logical_replication_workersin postgresql.conf.sample