I wrote:
> Oh, scratch that: I'd gotten confused about which branch I was
> working in. It does change the output as you say.
After poking at that for awhile, I decided we need a small tweak
in EXPLAIN itself to make the output consistent. See attached.
I'm now leaning against back-patching, as there is a user-visible
behavior change in EXPLAIN, and I don't think there are really
any severe consequences to the mistaken assignments.
regards, tom lane
From 19382e5a4af617a56e86d197f394bed93862cd66 Mon Sep 17 00:00:00 2001
From: Tom Lane <t...@sss.pgh.pa.us>
Date: Wed, 21 May 2025 12:00:38 -0400
Subject: [PATCH v1] In ExecInitModifyTable, don't scribble on the source plan.
The code carelessly modified mtstate->ps.plan->targetlist,
which it's not supposed to do. Fortunately, there's not
really any need to do that because the planner already
set up a perfectly acceptable targetlist for the plan node.
We just need to remove the erroneous assignments and update some
relevant comments.
As it happens, the erroneous assignments caused the targetlist to
point to a different part of the source plan tree, so that there
isn't really a risk of the pointer becoming dangling after executor
termination. The only visible effect we can find is that EXPLAIN will
show upper references to the ModifyTable's output expressions using
different variables. Formerly it showed Vars from the first target
relation that survived executor-startup pruning. Now it always shows
such references using the first relation appearing in the planner
output, independently of what happens during executor pruning. On the
whole that seems like a good thing. (We do need a small tweak in
ExplainPreScanNode to ensure that that relation will receive a refname
assignment in set_rtable_names, even if it got pruned at startup.)
Reported-by: Tom Lane <t...@sss.pgh.pa.us>
Author: Andres Freund <and...@anarazel.de>
Co-authored-by: Tom Lane <t...@sss.pgh.pa.us>
Discussion: https://postgr.es/m/213261.1747611...@sss.pgh.pa.us
---
src/backend/commands/explain.c | 4 ++
src/backend/executor/nodeModifyTable.c | 10 ++--
src/backend/optimizer/plan/setrefs.c | 7 +--
src/test/regress/expected/partition_prune.out | 47 ++++++++++---------
src/test/regress/sql/partition_prune.sql | 6 +--
5 files changed, 41 insertions(+), 33 deletions(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 786ee865f14..45942b4dd4f 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -1232,6 +1232,10 @@ ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
if (((ModifyTable *) plan)->exclRelRTI)
*rels_used = bms_add_member(*rels_used,
((ModifyTable *) plan)->exclRelRTI);
+ /* Ensure Vars used in RETURNING will have refnames */
+ if (plan->targetlist)
+ *rels_used = bms_add_member(*rels_used,
+ linitial_int(((ModifyTable *) plan)->resultRelations));
break;
case T_Append:
*rels_used = bms_add_members(*rels_used,
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 46d533b7288..2bc89bf84dc 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -4830,12 +4830,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
ExprContext *econtext;
/*
- * Initialize result tuple slot and assign its rowtype using the first
- * RETURNING list. We assume the rest will look the same.
+ * Initialize result tuple slot and assign its rowtype using the plan
+ * node's declared targetlist, which the planner set up to be the same
+ * as the first (before runtime pruning) RETURNING list. We assume
+ * all the result rels will produce compatible output.
*/
- mtstate->ps.plan->targetlist = (List *) linitial(returningLists);
-
- /* Set up a slot for the output of the RETURNING projection(s) */
ExecInitResultTupleSlotTL(&mtstate->ps, &TTSOpsVirtual);
slot = mtstate->ps.ps_ResultTupleSlot;
@@ -4865,7 +4864,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
* We still must construct a dummy result tuple type, because InitPlan
* expects one (maybe should change that?).
*/
- mtstate->ps.plan->targetlist = NIL;
ExecInitResultTypeTL(&mtstate->ps);
mtstate->ps.ps_ExprContext = NULL;
diff --git a/src/backend/optimizer/plan/setrefs.c b/src/backend/optimizer/plan/setrefs.c
index 150e9f060ee..8e1eb77dd49 100644
--- a/src/backend/optimizer/plan/setrefs.c
+++ b/src/backend/optimizer/plan/setrefs.c
@@ -1097,9 +1097,10 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
/*
* Set up the visible plan targetlist as being the same as
- * the first RETURNING list. This is for the use of
- * EXPLAIN; the executor won't pay any attention to the
- * targetlist. We postpone this step until here so that
+ * the first RETURNING list. This is mostly for the use
+ * of EXPLAIN; the executor won't execute that targetlist,
+ * although it does use it to prepare the node's result
+ * tuple slot. We postpone this step until here so that
* we don't have to do set_returning_clause_references()
* twice on identical targetlists.
*/
diff --git a/src/test/regress/expected/partition_prune.out b/src/test/regress/expected/partition_prune.out
index 0bf35260b46..d1966cd7d82 100644
--- a/src/test/regress/expected/partition_prune.out
+++ b/src/test/regress/expected/partition_prune.out
@@ -4553,16 +4553,18 @@ create view part_abc_view as select * from part_abc where b <> 'a' with check op
prepare update_part_abc_view as update part_abc_view set b = $2 where a = $1 returning *;
-- Only the unpruned partition should be shown in the list of relations to be
-- updated
-explain (costs off) execute update_part_abc_view (1, 'd');
- QUERY PLAN
--------------------------------------------------------
- Update on part_abc
- Update on part_abc_1
+explain (verbose, costs off) execute update_part_abc_view (1, 'd');
+ QUERY PLAN
+-----------------------------------------------------------------------------
+ Update on public.part_abc
+ Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
+ Update on public.part_abc_1
-> Append
Subplans Removed: 1
- -> Seq Scan on part_abc_1
- Filter: ((b <> 'a'::text) AND (a = $1))
-(6 rows)
+ -> Seq Scan on public.part_abc_1
+ Output: $2, part_abc_1.tableoid, part_abc_1.ctid
+ Filter: ((part_abc_1.b <> 'a'::text) AND (part_abc_1.a = $1))
+(8 rows)
execute update_part_abc_view (1, 'd');
a | b | c
@@ -4570,28 +4572,31 @@ execute update_part_abc_view (1, 'd');
1 | d | t
(1 row)
-explain (costs off) execute update_part_abc_view (2, 'a');
- QUERY PLAN
--------------------------------------------------------
- Update on part_abc
- Update on part_abc_2 part_abc_1
+explain (verbose, costs off) execute update_part_abc_view (2, 'a');
+ QUERY PLAN
+-----------------------------------------------------------------------------
+ Update on public.part_abc
+ Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
+ Update on public.part_abc_2
-> Append
Subplans Removed: 1
- -> Seq Scan on part_abc_2 part_abc_1
- Filter: ((b <> 'a'::text) AND (a = $1))
-(6 rows)
+ -> Seq Scan on public.part_abc_2
+ Output: $2, part_abc_2.tableoid, part_abc_2.ctid
+ Filter: ((part_abc_2.b <> 'a'::text) AND (part_abc_2.a = $1))
+(8 rows)
execute update_part_abc_view (2, 'a');
ERROR: new row violates check option for view "part_abc_view"
DETAIL: Failing row contains (2, a, t).
-- All pruned.
-explain (costs off) execute update_part_abc_view (3, 'a');
- QUERY PLAN
------------------------------
- Update on part_abc
+explain (verbose, costs off) execute update_part_abc_view (3, 'a');
+ QUERY PLAN
+----------------------------------------------------
+ Update on public.part_abc
+ Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
-> Append
Subplans Removed: 2
-(3 rows)
+(4 rows)
execute update_part_abc_view (3, 'a');
a | b | c
diff --git a/src/test/regress/sql/partition_prune.sql b/src/test/regress/sql/partition_prune.sql
index f6db9479f54..d93c0c03bab 100644
--- a/src/test/regress/sql/partition_prune.sql
+++ b/src/test/regress/sql/partition_prune.sql
@@ -1371,12 +1371,12 @@ create view part_abc_view as select * from part_abc where b <> 'a' with check op
prepare update_part_abc_view as update part_abc_view set b = $2 where a = $1 returning *;
-- Only the unpruned partition should be shown in the list of relations to be
-- updated
-explain (costs off) execute update_part_abc_view (1, 'd');
+explain (verbose, costs off) execute update_part_abc_view (1, 'd');
execute update_part_abc_view (1, 'd');
-explain (costs off) execute update_part_abc_view (2, 'a');
+explain (verbose, costs off) execute update_part_abc_view (2, 'a');
execute update_part_abc_view (2, 'a');
-- All pruned.
-explain (costs off) execute update_part_abc_view (3, 'a');
+explain (verbose, costs off) execute update_part_abc_view (3, 'a');
execute update_part_abc_view (3, 'a');
deallocate update_part_abc_view;
--
2.43.5
