Thread: [HACKERS] scan on inheritance parent with no children in current session

Hi All,
Consider a parent table which has no child in the current session, but
has temporary children in other sessions.

Session 1
postgres=# create table parent (a int);
CREATE TABLE

Session 2:
postgres=# create temp table temp_child () inherits(parent);
CREATE TABLE

Before commit d3cc37f1d801a6b5cad9bf179274a8d767f1ee50. We would have
Seq Scan plan for scanning parent in session 1
postgres=# explain verbose select * from parent;
                         QUERY PLAN
-------------------------------------------------------------
 Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=4)
   Output: parent.a
(2 rows)

In session 2, it would be an Append plan
postgres=# explain verbose select * from parent;
                                  QUERY PLAN
------------------------------------------------------------------------------
 Append  (cost=0.00..35.50 rows=2551 width=4)
   ->  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=4)
         Output: parent.a
   ->  Seq Scan on pg_temp_4.temp_child  (cost=0.00..35.50 rows=2550 width=4)
         Output: temp_child.a
(5 rows)

After that commit in session 1, we get an Append plan
postgres=# explain verbose select * from parent;
                            QUERY PLAN
-------------------------------------------------------------------
 Append  (cost=0.00..0.00 rows=1 width=4)
   ->  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=4)
         Output: parent.a
(3 rows)

I don't think this is an intentional change. Here's patch to fix it.
The comment in the patch uses term "real child" in the context of
comments about temporary children from other session and the comment
at the end of the function where rte->inh is reset. May be we should
move the second comment before setting has_child in the patch and use
"real child" in the comment at the end to avoid repetition. But I want
to first check whether we want this fix or we can live with the Append
plan.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2017/08/04 18:11, Ashutosh Bapat wrote:
> After that commit in session 1, we get an Append plan
> postgres=# explain verbose select * from parent;
>                             QUERY PLAN
> -------------------------------------------------------------------
>  Append  (cost=0.00..0.00 rows=1 width=4)
>    ->  Seq Scan on public.parent  (cost=0.00..0.00 rows=1 width=4)
>          Output: parent.a
> (3 rows)
> 
> I don't think this is an intentional change. Here's patch to fix it.
> The comment in the patch uses term "real child" in the context of
> comments about temporary children from other session and the comment
> at the end of the function where rte->inh is reset. May be we should
> move the second comment before setting has_child in the patch and use
> "real child" in the comment at the end to avoid repetition. But I want
> to first check whether we want this fix or we can live with the Append
> plan.

Good catch.  I agree that getting an Append node after that commit is
unintentional and we should fix so that we don't get an Append.  So, +1 to
your patch.  I looked at the patch and the code fix seems to do what we want.

Thanks,
Amit

On Sun, Aug 6, 2017 at 10:56 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Good catch.  I agree that getting an Append node after that commit is
> unintentional and we should fix so that we don't get an Append.  So, +1 to
> your patch.  I looked at the patch and the code fix seems to do what we want.

So, I also agree that this is a good fix, but I don't think it fixes
the whole problem.  Consider:

rhaas=# create table parent (a int) partition by list (a);
CREATE TABLE
rhaas=# create temp table child partition of parent for values in (1);
CREATE TABLE
rhaas=# explain verbose select * from parent;                              QUERY PLAN
-------------------------------------------------------------------------Append  (cost=0.00..35.50 rows=2550 width=4)
-> Seq Scan on pg_temp_3.child  (cost=0.00..35.50 rows=2550 width=4)        Output: child.a
 
(3 rows)

But the comments say:
* A childless table is never considered to be an inheritance set; therefore* a parent RTE must always have at least two
associatedAppendRelInfos.
 

Yet, not.  So at least the comments need to be updated; not sure if we
want to try to eliminate the Append node in this case also.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Aug 11, 2017 at 9:08 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> rhaas=# create table parent (a int) partition by list (a);
> CREATE TABLE
> rhaas=# create temp table child partition of parent for values in (1);
> CREATE TABLE
> rhaas=# explain verbose select * from parent;
>                                QUERY PLAN
> -------------------------------------------------------------------------
>  Append  (cost=0.00..35.50 rows=2550 width=4)
>    ->  Seq Scan on pg_temp_3.child  (cost=0.00..35.50 rows=2550 width=4)
>          Output: child.a
> (3 rows)
>
> But the comments say:
>
>  * A childless table is never considered to be an inheritance set; therefore
>  * a parent RTE must always have at least two associated AppendRelInfos.
>
> Yet, not.  So at least the comments need to be updated;

A partitioned table with at least a single partition is not childless.
So, above comment doesn't apply here. In a regular inheritance there
will be at least two AppendRelInfos one representing a scan on the
parent table and others representing the scan on children. The comment
needs to be distinguish between regular inheritance and partitioned
inheritance. I have modified the comments that way.

> not sure if we
> want to try to eliminate the Append node in this case also.
>

The rest of the query tree and plan tree, expects parent's targetlist
which may be different from that of the child. A notable difference is
whole row expressions getting translated using ConvertRowExpr, which
are not expected to pop up in a scan's targetlist. So we can't
eliminate the Append node.

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

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Aug 14, 2017 at 1:49 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> I have modified the comments that way.

Committed with some cleanup.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company