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

On 2019/04/19 4:41, Tom Lane wrote:
> 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.

Doesn't "Update on public.parent" in the first line serve that purpose
though?  It identifies exactly the table whose statement triggers will be
fired.  IOW, we are still listing the only table that the execution is
going to do something meaningful with as part of this otherwise empty command.

>> 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.

Hmm, I'm thinking that showing the table name prefix would be better at
least in this case, because otherwise it's unclear whose columns the
Result node is showing; with the prefix, it's clear they are parent's.

However, it seems that for the same case but with a partitioned table
target, the prefix is not shown even with the patch:

create table p (a int, b text) partition by list (a);
create table p1 partition of p for values in (1);
explain verbose update p set a = 1 where false returning *;
                      QUERY PLAN
──────────────────────────────────────────────────────
 Update on public.p  (cost=0.00..0.00 rows=0 width=0)
   Output: a, b
   ->  Result  (cost=0.00..0.00 rows=0 width=0)
         Output: 1, b, ctid
         One-Time Filter: false
(5 rows)

...but for a totally unrelated reason.  At least in HEAD, there's only one
entry in the range table -- the original target table -- and no entries
for partitions due to our recent surgery of inheritance planning.
explain.c: show_plan_tlist() has the following rule to determine whether
to show the prefix:

    useprefix = list_length(es->rtable) > 1;

Same rule applies to the case where the target relation is a regular table.

I guess such a discrepancy is not good.

> 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.

Yeah, I missed updating this and maybe some other comments about
nominalRelation.

> One idea, perhaps, is to teach explain.c to not list partitioned tables
> as subsidiary targets, independently of nominalRelation.

We don't list partitioned tables as subsidiary targets to begin with,
because they're not added to resultRelations list, where the subsidiary
targets that are listed come from.

> 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.

Note that I'm only talking about the case in which all result relations
have been excluded, *irrespective* of whether the result relation is a
regular inherited table or a partitioned table, which the commit at the
head of this discussion thread addressed in all branches.  The only
difference between the the regular inheritance target relation case and
partitioned table target relation case is that nominalRelation refers to
the first child member relation for the former, whereas it refers to the
original target relation for the latter.  Given that difference, the
problem I'm addressing with the proposed patch doesn't occur when the
target relation is a partitioned table.

In a way, what we implicitly aimed for with the previously committed fix
for the empty UPDATE case is that the resulting plan and execution
behavior is same for all 3 types of target relations: regular table
(handled totally in grouping_planner()), regular inherited table, and
partitioned table (the latter two handled in inheritance_planner()).  Per
my complaint here, that is not totally the case, which is simply a result
of differences in the underlying implementation of those three cases, some
of which I've mentioned above.

-- on HEAD (and tips of other branches)
create table foo (a int);
create table foo1 () inherits (foo);
create table p (a int) partition by list (a);
create table p1 partition of p for values in (1);

-- only this case shows subsidiary target relation
explain verbose update foo set a = 1 where false returning *;
                       QUERY PLAN
────────────────────────────────────────────────────────
 Update on public.foo  (cost=0.00..0.00 rows=0 width=0)
   Output: a
   Update on public.foo
   ->  Result  (cost=0.00..0.00 rows=0 width=0)
         Output: 1, ctid
         One-Time Filter: false
(6 rows)

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

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

Now, there's a showstopper to my proposed patch (targetlists being
displayed differently in different cases as illustrated above), but I do
think we should try to make the output uniform in some way.

Thanks,
Amit