Re: pgsql: Clarify use of temporary tables within partition trees - Mailing list pgsql-committers

On 2018/07/03 18:05, David Rowley wrote:
> (re-adding committers list)
> 
> On 3 July 2018 at 20:31, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> 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...
> 
> That will cause the AppendRelInfo not to be built for a partitioned
> table with no partitions. I'm not personally certain that doing that
> comes without consequence.  Are you certain of that?  If not 100%,
> then I think that's more of a task for PG12. I'm trying to propose
> removing dead code for PG11 and on.

What to use the AppendRelInfo for if there is no partition?

> My current thoughts are that moving this test up a few lines is
> unlikely to improve performance in a real-world situation, so it does
> not seem worth the risk for a backpatch to me.

It's just that needless structures get created which we could avoid, and
once I realized that, I found out about the 2 below.

If you look at all of what expand_single_inheritance_child does, I'm a bit
surprised you don't think we should try to avoid calling it if we don't
need any of that processing.

>> 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.
> 
> I recently saw this too and wondered about it. I then wrote a patch to
> just have it create a single RangeTblEntry for each partitioned table.
> The tests all passed.

Which is how I was thinking the commit 0a480502b09 had done it, but
apparently not.  That's a commit in PG11.  None of what I'm suggesting is
for PG 10.

> I'd categorise this one the same as I have #1 above, i.e. not
> backpatch material. It seems like something useful to look into for
> v12 though. I assumed this was done for a reason and that I just
> didn't understand what that reason was. I don't recall any comments to
> explain the reason why we build two RangeTblEntrys for each
> partitioned table.

Maybe because that's what's done for the root parent in a plain
inheritance hierarchy, which is always a plain table.  In that case, one
RTE is for its role as the parent (with rte->inh = true) and another is
for its role as a child (with rte->inh = false).  The former is processed
as an append rel and the latter as a plain rel that will get assigned scan
paths such as SeqScan, etc.

For partitioned table parent(s), we need only the former because they can
only be processed as append rels.  That's why I'm proposing we could
adjust the commit in PG 11 that introduced expand_partitioned_rtentry such
that the duplicate child RTE and other objects are not created.

Thanks,
Amit