Re: pgsql: Fix plan created for inherited UPDATE/DELETE with alltables exc

On 2019/02/23 2:23, Tom Lane wrote:
> Fix plan created for inherited UPDATE/DELETE with all tables excluded.
> 
> In the case where inheritance_planner() finds that every table has
> been excluded by constraints, it thought it could get away with
> making a plan consisting of just a dummy Result node.  While certainly
> there's no updating or deleting to be done, this had two user-visible
> problems: the plan did not report the correct set of output columns
> when a RETURNING clause was present, and if there were any
> statement-level triggers that should be fired, it didn't fire them.
> 
> Hence, rather than only generating the dummy Result, we need to
> stick a valid ModifyTable node on top, which requires a tad more
> effort here.
> 
> It's been broken this way for as long as inheritance_planner() has
> known about deleting excluded subplans at all (cf commit 635d42e9c),
> so back-patch to all supported branches.

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.  It's shown because nominalRelation
doesn't match resultRelation, which prompts explain.c to to print the
child target relations separately per this code in show_modifytable_info():

    /* Should we explicitly label target relations? */
    labeltargets = (mtstate->mt_nplans > 1 ||
                    (mtstate->mt_nplans == 1 &&
                     mtstate->resultRelInfo->ri_RangeTableIndex !=
node->nominalRelation));

Attached a patch to adjust nominalRelation in this case so that "parent"
won't be shown a second time, as follows:

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

As you may notice, Result node's targetlist is shown differently than
before, that is, columns are shown prefixed with table name.  In the old
output, the prefix or "refname"  ends up NULL, because the Result node's
targetlist uses resultRelation (1) as varno, which is not referenced
anywhere in the plan tree (for ModifyTable, explain.c prefers to use
nominalRelation instead of resultRelation).  With patch applied,
nominalRelation == resultRelation, so it is referenced and hence its
"refname" non-NULL.  Maybe this change is fine though?

Thanks,
Amit