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

On Sat, Sep 9, 2017 at 6:28 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Sep 8, 2017 at 1:38 PM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>>> I also confirmed that the partition-pruning patch set works fine with this
>>> patch instead of the patch on that thread with the same functionality,
>>> which I will now drop from that patch set. Sorry about the wasted time.
>>
>> Thanks a lot. Please review the patch in the updated patchset.
>
> In set_append_rel_size(), I don't find the comment too clear (and this
> part was taken from Amit's patch, right?). I suggest:

No, I didn't take it from Amit's patch. Both of us have different
wordings. But yours is better than both of us. Included it in the
attached patches.

>
> + /*
> + * Associate the partitioned tables which are descendents of the table
> + * named in the query with the topmost append path (i.e. the one where
> + * rel->reloptkind is RELOPT_BASEREL). This ensures that they get properly
> + * locked at execution time.
> + */
>
> I'm a bit suspicious about the fact that there are now executor
> changes related to the PlanRowMarks. If the rowmark's prti is now the
> intermediate parent's RT index rather than the top-parent's RT index,
> it'd seem like that'd matter somehow. Maybe it doesn't, because the
> code that cares about prti seems to only care about whether it's
> different from rti. But if that's true everywhere, then why even
> change this? I think we might be well off not to tinker with things
> that don't need to be changed.

In the definition of ExecRowMark, I see
Index prti; /* parent range table index, if child */
It just says parent, by which I take as direct parent. For
inheritance, which earlier flattened inheritance hierarchy, direct
parent was top parent. So, probably nobody thought whether a parent is
direct parent or top parent. But now that we have introduced that
concept we need to interpret this comment anew. And I think
interpreting it as direct parent is non-lossy. If we set top parent's
index, parent RTI in AppendRelInfo and PlanRowMark would not agree.
So, it looks quite natural that we set the direct parent's index in
PlanRowMark. From that POV, we aren't changing anything, we are
setting the same parent RTI in AppendRelInfo and PlanRowMark. Chaning
different parent RTIs in those two structure would be a real change.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company