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

On Fri, Sep 8, 2017 at 12:34 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Sep 5, 2017 at 7:01 AM, Ashutosh Bapat
> <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>> accumulate_append_subpath() is executed for every path instead of
>> every relation, so changing it would collect the same list multiple
>> times. Instead, I found the old way of associating all intermediate
>> partitions with the root partitioned relation work better. Here's the
>> updated patch set.
>
> When I tried out patch 0001, it crashed repeatedly during 'make check'
> because of an assertion failure in get_partitioned_child_rels.

Running "make check" on the whole patchset doesn't give that failure.
So I didn't notice the crash since I was running regression on the
whole patchset. Sorry for that. Fortunately git rebase -i allows us to
execute shell commands while applying patches, so I have set it up to
compile each patch and run regression. Hopefully that will catch such
errors in future. That process showed me that patch
0003-In-add_paths_to_append_rel-get-partitioned_rels-for-.patch fixes
that crash by calling get_partitioned_child_rels() only on the root
partitioned table for which we have set up child_rels list. Amit
Langote has a similar fix reported in his reply. So, we will discuss
it there.

> It
> seemed to me that the way the patch was refactoring
> expand_inherited_rtentry involved more code rearrangement than
> necessary, so I reverted all the code rearrangement and just kept the
> functional changes, and all the crashes went away. (That refactoring
> also failed to initialize has_child properly.) In so doing, I
> reintroduced the problem that the PartitionedChildRelInfo lists
> weren't getting set up correctly, but after some thought I realized
> that was just because expand_single_inheritance_child() was choosing
> between adding an RTE and adding the OID to partitioned_child_rels,
> whereas for an intermediate partitioned table it needs to do both. So
> I inserted a trivial fix for that problem (replacing "else" with a new
> "if"-test), basically:
>
> - else
> +
> + if (childrte->relkind == RELKIND_PARTITIONED_TABLE)
>
> Please check out the attached version of the patch. In addition to
> the above simplifications, I did some adjustments to the comments in
> various places - some just grammar and others a bit more substantive.
> And I think I broke a long line in one place, too.
>
> One thing I notice is that if I rip out the changes to initsplan.c,
> the new regression test still passes. If it's possible to write a
> test that fails without those changes, I think it would be a good idea
> to include one in the patch. That's certainly one of the subtler
> parts of this patch, IMHO.

Amit Langote has replied on these points as well. So, I will comment
in a reply to his reply.

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