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

On Mon, Jul 31, 2017 at 7:59 AM, Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> Adding AppendRelInfos to root->append_rel_list as and when they are
> created would keep parent AppendRelInfos before those of children. But
> that function throws away the AppendRelInfo it created when their are
> no real children i.e. in partitioned table's case when has no leaf
> partitions. So, we can't do that. Hence, I chose to change the API to
> return the list of AppendRelInfos when the given RTE has real
> children.

So, IIUC, the case you're concerned about is when you have a hierarchy
of only partitioned tables, with no plain tables. For example, if B
is a partitioned table and a partition of A, and that's all there is,
A will recurse to B and B will return NIL.

Is it necessary to get rid of the extra AppendRelInfos, or are they
harmless like the duplicate RTE and PlanRowMark nodes?

/*
* If all the children were temp tables or a partitioned parent did not
* have any leaf partitions, pretend it's a non-inheritance situation; we
* don't need Append node in that case. The duplicate RTE we added for
* the parent table is harmless, so we don't bother to get rid of it;
* ditto for the useless PlanRowMark node.
*/
if (!need_append)
{
/* Clear flag before returning */
rte->inh = false;
return;
}

If we do need to get rid of the extra AppendRelInfos, maybe a less
invasive solution would be to change the if (!need_append) case to do
root->append_rel_list = list_truncate(root->append_rel_list,
original_append_rel_length).

> The code avoids creating AppendRelInfos for a child which represents
> the parent in its role as a simple member of inheritance set.

OK, I suggest we rewrite the whole comment like this: "We need an
AppendRelInfo if paths will be built for the child RTE. If
childrte->inh is true, then we'll always need to generate append paths
for it. If childrte->inh is false, we must scan it if it's not a
partitioned table; but if it is a partitioned table, then it never has
any data of its own and need not be scanned. It does, however, need
to be locked, so note the OID for inclusion in the
PartitionedChildRelInfo we're going to build."

It looks like you also need to update the header comment for
AppendRelInfo itself, in nodes/relation.h.

+ * PlannerInfo for every child is obtained by translating relevant members

Insert "The" at the start of the sentence.

- subroot->parse = (Query *)
- adjust_appendrel_attrs(root,
- (Node *) parse,
- appinfo);
+ subroot->parse = (Query *) adjust_appendrel_attrs(parent_root,
+ (Node *)
parent_parse,
+ 1, &appinfo);

I suggest that you don't remove the line break after the cast.

+ * If the child is further partitioned, remember it as a parent. Since
+ * partitioned tables do not have any data, we don't need to create
+ * plan for it. We just need its PlannerInfo set up to be used while
+ * planning its children.

Most of this comment is in the singular, but the first half of the
second sentence is plural. Should be "Since a partitioned table does
not have any data...". I might replace the last sentence by "We do,
however, need to remember the PlannerInfo for use when planning its
children."

+-- Check UPDATE with *multi-level partitioned* inherited target

Asterisks seem like overkill.

Since expand_inherited_rtentry() and set_append_rel_size() can now
recurse down to as many levels as there are levels in the inheritance
hierarchy, they should probably have a check_stack_depth() check.

>> 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.
>>
> Planner code is already aware of such hierarchies except DMLs, which
> this patch adjusts. We have fixed issues revealed by mine and
> Rajkumar's testing.
> What kinds of things you suspect?

I'm not sure exactly. It's just hard with this kind of patch to make
sure you've caught everything.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company