Re: pgsql: Clarify use of temporary tables within partition trees

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