Re: Partition-wise join for join between (declaratively) partitioned tables

On Fri, Jul 14, 2017 at 3:02 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)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 all the 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