Thread: pgsql: Fix plan created for inherited UPDATE/DELETE with all tablesexc
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. Amit Langote and Tom Lane, per a report from Petr Fedorov. Discussion: https://postgr.es/m/5da6f0f0-1364-1876-6978-907678f89a3e@phystech.edu Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ab5fcf2b04f9cc4ecccb1832faabadb047087d23 Modified Files -------------- src/backend/optimizer/plan/planner.c | 68 +++++++++++++++++++++++----------- src/test/regress/expected/inherit.out | 41 ++++++++++++++++++++ src/test/regress/expected/triggers.out | 34 +++++++++++++++++ src/test/regress/sql/inherit.sql | 15 ++++++++ src/test/regress/sql/triggers.sql | 27 ++++++++++++++ 5 files changed, 163 insertions(+), 22 deletions(-)
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
Attachment
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
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
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
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
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