Skip to content

Commit d376ab5

Browse files
tglsfdctglsfdc
committed
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 of this change 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. Also make a small tweak in ExplainPreScanNode to ensure that the first relation will receive a refname assignment in set_rtable_names, even if it got pruned at startup. Previously the Vars might be shown without any table qualification, which is confusing in a multi-table query. I considered back-patching this, but since the bug doesn't seem to have any really terrible consequences in existing branches, it seems better to not change their EXPLAIN output. It's not too late for v18 though, especially since v18 already made other changes in the EXPLAIN output for these cases. Reported-by: Tom Lane <tgl@sss.pgh.pa.us> Author: Andres Freund <andres@anarazel.de> Co-authored-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/213261.1747611093@sss.pgh.pa.us
1 parent f24605e commit d376ab5

File tree

5 files changed

+41
-33
lines changed

5 files changed

+41
-33
lines changed

‎src/backend/commands/explain.c

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1220,6 +1220,10 @@ ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
12201220
if (((ModifyTable *) plan)->exclRelRTI)
12211221
*rels_used = bms_add_member(*rels_used,
12221222
((ModifyTable *) plan)->exclRelRTI);
1223+
/* Ensure Vars used in RETURNING will have refnames */
1224+
if (plan->targetlist)
1225+
*rels_used = bms_add_member(*rels_used,
1226+
linitial_int(((ModifyTable *) plan)->resultRelations));
12231227
break;
12241228
case T_Append:
12251229
*rels_used = bms_add_members(*rels_used,

‎src/backend/executor/nodeModifyTable.c

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -4830,12 +4830,11 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
48304830
ExprContext *econtext;
48314831

48324832
/*
4833-
* Initialize result tuple slot and assign its rowtype using the first
4834-
* RETURNING list. We assume the rest will look the same.
4833+
* Initialize result tuple slot and assign its rowtype using the plan
4834+
* node's declared targetlist, which the planner set up to be the same
4835+
* as the first (before runtime pruning) RETURNING list. We assume
4836+
* all the result rels will produce compatible output.
48354837
*/
4836-
mtstate->ps.plan->targetlist = (List *) linitial(returningLists);
4837-
4838-
/* Set up a slot for the output of the RETURNING projection(s) */
48394838
ExecInitResultTupleSlotTL(&mtstate->ps, &TTSOpsVirtual);
48404839
slot = mtstate->ps.ps_ResultTupleSlot;
48414840

@@ -4865,7 +4864,6 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags)
48654864
* We still must construct a dummy result tuple type, because InitPlan
48664865
* expects one (maybe should change that?).
48674866
*/
4868-
mtstate->ps.plan->targetlist = NIL;
48694867
ExecInitResultTypeTL(&mtstate->ps);
48704868

48714869
mtstate->ps.ps_ExprContext = NULL;

‎src/backend/optimizer/plan/setrefs.c

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1097,9 +1097,10 @@ set_plan_refs(PlannerInfo *root, Plan *plan, int rtoffset)
10971097

10981098
/*
10991099
* Set up the visible plan targetlist as being the same as
1100-
* the first RETURNING list. This is for the use of
1101-
* EXPLAIN; the executor won't pay any attention to the
1102-
* targetlist. We postpone this step until here so that
1100+
* the first RETURNING list. This is mostly for the use
1101+
* of EXPLAIN; the executor won't execute that targetlist,
1102+
* although it does use it to prepare the node's result
1103+
* tuple slot. We postpone this step until here so that
11031104
* we don't have to do set_returning_clause_references()
11041105
* twice on identical targetlists.
11051106
*/

‎src/test/regress/expected/partition_prune.out

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -4553,45 +4553,50 @@ create view part_abc_view as select * from part_abc where b <> 'a' with check op
45534553
prepare update_part_abc_view as update part_abc_view set b = $2 where a = $1 returning *;
45544554
-- Only the unpruned partition should be shown in the list of relations to be
45554555
-- updated
4556-
explain (costs off) execute update_part_abc_view (1, 'd');
4557-
QUERY PLAN
4558-
-------------------------------------------------------
4559-
Update on part_abc
4560-
Update on part_abc_1
4556+
explain (verbose, costs off) execute update_part_abc_view (1, 'd');
4557+
QUERY PLAN
4558+
-----------------------------------------------------------------------------
4559+
Update on public.part_abc
4560+
Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
4561+
Update on public.part_abc_1
45614562
-> Append
45624563
Subplans Removed: 1
4563-
-> Seq Scan on part_abc_1
4564-
Filter: ((b <> 'a'::text) AND (a = $1))
4565-
(6 rows)
4564+
-> Seq Scan on public.part_abc_1
4565+
Output: $2, part_abc_1.tableoid, part_abc_1.ctid
4566+
Filter: ((part_abc_1.b <> 'a'::text) AND (part_abc_1.a = $1))
4567+
(8 rows)
45664568

45674569
execute update_part_abc_view (1, 'd');
45684570
a | b | c
45694571
---+---+---
45704572
1 | d | t
45714573
(1 row)
45724574

4573-
explain (costs off) execute update_part_abc_view (2, 'a');
4574-
QUERY PLAN
4575-
-------------------------------------------------------
4576-
Update on part_abc
4577-
Update on part_abc_2 part_abc_1
4575+
explain (verbose, costs off) execute update_part_abc_view (2, 'a');
4576+
QUERY PLAN
4577+
-----------------------------------------------------------------------------
4578+
Update on public.part_abc
4579+
Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
4580+
Update on public.part_abc_2
45784581
-> Append
45794582
Subplans Removed: 1
4580-
-> Seq Scan on part_abc_2 part_abc_1
4581-
Filter: ((b <> 'a'::text) AND (a = $1))
4582-
(6 rows)
4583+
-> Seq Scan on public.part_abc_2
4584+
Output: $2, part_abc_2.tableoid, part_abc_2.ctid
4585+
Filter: ((part_abc_2.b <> 'a'::text) AND (part_abc_2.a = $1))
4586+
(8 rows)
45834587

45844588
execute update_part_abc_view (2, 'a');
45854589
ERROR: new row violates check option for view "part_abc_view"
45864590
DETAIL: Failing row contains (2, a, t).
45874591
-- All pruned.
4588-
explain (costs off) execute update_part_abc_view (3, 'a');
4589-
QUERY PLAN
4590-
-----------------------------
4591-
Update on part_abc
4592+
explain (verbose, costs off) execute update_part_abc_view (3, 'a');
4593+
QUERY PLAN
4594+
----------------------------------------------------
4595+
Update on public.part_abc
4596+
Output: part_abc_1.a, part_abc_1.b, part_abc_1.c
45924597
-> Append
45934598
Subplans Removed: 2
4594-
(3 rows)
4599+
(4 rows)
45954600

45964601
execute update_part_abc_view (3, 'a');
45974602
a | b | c

‎src/test/regress/sql/partition_prune.sql

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1371,12 +1371,12 @@ create view part_abc_view as select * from part_abc where b <> 'a' with check op
13711371
prepare update_part_abc_view as update part_abc_view set b = $2 where a = $1 returning *;
13721372
-- Only the unpruned partition should be shown in the list of relations to be
13731373
-- updated
1374-
explain (costs off) execute update_part_abc_view (1, 'd');
1374+
explain (verbose, costs off) execute update_part_abc_view (1, 'd');
13751375
execute update_part_abc_view (1, 'd');
1376-
explain (costs off) execute update_part_abc_view (2, 'a');
1376+
explain (verbose, costs off) execute update_part_abc_view (2, 'a');
13771377
execute update_part_abc_view (2, 'a');
13781378
-- All pruned.
1379-
explain (costs off) execute update_part_abc_view (3, 'a');
1379+
explain (verbose, costs off) execute update_part_abc_view (3, 'a');
13801380
execute update_part_abc_view (3, 'a');
13811381
deallocate update_part_abc_view;
13821382

0 commit comments

Comments
 (0)