On 2018/07/03 17:31, Amit Langote wrote:
> On 2018/07/03 16:05, Michael Paquier wrote:
>> On Tue, Jul 03, 2018 at 03:49:44PM +0900, Amit Langote wrote:
>>> I forgot that expand_partitioned_rtentry() will recursively call itself if
>>> a partition is itself a partitioned table, in which case the above
>>> code helps.
>>
>> Actually look at the coverage reports:
>> https://coverage.postgresql.org/src/backend/optimizer/prep/prepunion.c.gcov.html
>> 1742 : /*
>> 1743 : * If the partitioned table has no partitions or all the partitions are
>> 1744 : * temporary tables from other backends, treat this as non-inheritance
>> 1745 : * case.
>> 1746 : */
>> 1747 4920 : if (!has_child)
>> 1748 0 : parentrte->inh = false;
>> 1749 4920 : }
>>
>> expand_partitioned_rtentry() never disables this flag on recursive calls
>> with a multi-level tree. Could it be possible to get a test which
>> closes the gap?
>
> I guess that it will be hard as you mentioned before on the thread that
> led to this commit. We can't write regression tests which require using
> temporary partitions from other sessions.
>
> Anyway, I just wanted to say that I was wrong when I said that the block
> added by the patch is unreachable. It *is* reachable for multi-level
> partitioning. For example, it will execute in the following case:
>
> create table p (a int, b int) partition by list (a);
> create table pd partition of p default partition by list (b);
> select * from p;
>
> expand_partitioned_rtentry will get called twice and the newly added code
> would result in early return from the function in the 2nd invocation which
> is for 'pd'.
>
> But,
>
> 1. I still insist that it's better for the newly added code to be near the
> top of the function body than in the middle, which brings me to...
>
> 2. While we're at fixing the code around here, I think we should think
> about trying to get rid of the *non-dead* code that produces a structure
> that isn't used anywhere, which I was under the impression, 0a480502b09
> [1] already did (cc'ing Ashutosh). To clarify, we still unnecessarily
> create a "duplicate" RTE for partitioned tables in a partition tree
> (non-leaf tables) in its role as a child. So, for the above query, there
> end up being created 4 entries in the query's range table (2 for 'p' and 2
> for 'pd'). That makes sense for plain inheritance, because even the root
> parent table in a plain inheritance tree is a regular table containing
> data. That's not true for partition inheritance trees, where non-leaf
> tables contain no data, so we don't create a plan to scan them (see
> d3cc37f1d80 [2]), which in turn means we don't need to create the
> redundant "duplicate" child RTEs for them either.
>
> See attached my delta patch to address both 1 and 2.
>
> Thanks,
> Amit
>
> [1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0a480502b09
>
> [2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d3cc37f1d80
For some reason, ML address got removed from the list of address when
sending the above message.
Thanks,
Amit
