Re: pgsql: Fix plan created for inherited UPDATE/DELETE with all tables exc - Mailing list pgsql-committers

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/02/23 2:23, Tom Lane wrote:
>> Fix plan created for inherited UPDATE/DELETE with all tables excluded.

> I noticed that we may have forgotten to fix one more thing in this commit
> -- nominalRelation may refer to a range table entry that's not referenced
> elsewhere in the finished plan:

> create table parent (a int);
> create table child () inherits (parent);
> explain verbose update parent set a = a where false;
>                         QUERY PLAN
> ───────────────────────────────────────────────────────────
>  Update on public.parent  (cost=0.00..0.00 rows=0 width=0)
>    Update on public.parent
>    ->  Result  (cost=0.00..0.00 rows=0 width=0)
>          Output: a, ctid
>          One-Time Filter: false
> (5 rows)

> I think the "Update on public.parent" shown in the 2nd row is unnecessary,
> because it won't really be updated.

Well, perhaps, but nonetheless that's what the plan tree shows.
Moreover, even though it will receive no row changes, we're going to fire
statement-level triggers on it.  So I'm not entirely convinced that
suppressing it is a step forward ...

> As you may notice, Result node's targetlist is shown differently than
> before, that is, columns are shown prefixed with table name.

... and that definitely isn't one.

I also think that your patch largely falsifies the discussion at 1543ff:

         * Set the nominal target relation of the ModifyTable node if not
         * already done.  If the target is a partitioned table, we already set
         * nominalRelation to refer to the partition root, above.  For
         * non-partitioned inheritance cases, we'll use the first child
         * relation (even if it's excluded) as the nominal target relation.
         * Because of the way expand_inherited_rtentry works, that should be
         * the RTE representing the parent table in its role as a simple
         * member of the inheritance set.
         *
         * It would be logically cleaner to *always* use the inheritance
         * parent RTE as the nominal relation; but that RTE is not otherwise
         * referenced in the plan in the non-partitioned inheritance case.
         * Instead the duplicate child RTE created by expand_inherited_rtentry
         * is used elsewhere in the plan, so using the original parent RTE
         * would give rise to confusing use of multiple aliases in EXPLAIN
         * output for what the user will think is the "same" table.  OTOH,
         * it's not a problem in the partitioned inheritance case, because
         * there is no duplicate RTE for the parent.

We'd have to rethink exactly what the goals are if we want to change
the definition of nominalRelation like this.

One idea, perhaps, is to teach explain.c to not list partitioned tables
as subsidiary targets, independently of nominalRelation.  But I'm not
convinced that we need to do anything at all here.  The existing output
for this case is exactly like it was in v10 and v11.

            regards, tom lane