Skip to content

Commit 99a1554

Browse files
tglsfdctglsfdc
committed
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 635d42e), 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
1 parent 5a73edf commit 99a1554

File tree

5 files changed

+150
-17
lines changed

5 files changed

+150
-17
lines changed

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

Lines changed: 35 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,29 +1087,47 @@ inheritance_planner(PlannerInfo *root)
10871087
/* Mark result as unordered (probably unnecessary) */
10881088
root->query_pathkeys = NIL;
10891089

1090-
/*
1091-
* If we managed to exclude every child rel, return a dummy plan; it
1092-
* doesn't even need a ModifyTable node.
1093-
*/
10941090
if (subplans == NIL)
10951091
{
1096-
/* although dummy, it must have a valid tlist for executor */
1092+
/*
1093+
* We managed to exclude every child rel, so generate a dummy path
1094+
* representing the empty set. Although it's clear that no data will
1095+
* be updated or deleted, we will still need to have a ModifyTable
1096+
* node so that any statement triggers are executed. (This could be
1097+
* cleaner if we fixed nodeModifyTable.c to support zero child nodes,
1098+
* but that probably wouldn't be a net win.)
1099+
*/
10971100
List *tlist;
1101+
Plan *subplan;
10981102

1103+
/* tlist processing never got done, either */
10991104
tlist = preprocess_targetlist(root, parse->targetList);
1100-
return (Plan *) make_result(root,
1101-
tlist,
1102-
(Node *) list_make1(makeBoolConst(false,
1103-
false)),
1104-
NULL);
1105-
}
11061105

1107-
/*
1108-
* Put back the final adjusted rtable into the master copy of the Query.
1109-
*/
1110-
parse->rtable = final_rtable;
1111-
root->simple_rel_array_size = save_rel_array_size;
1112-
root->simple_rel_array = save_rel_array;
1106+
subplan = (Plan *) make_result(root,
1107+
tlist,
1108+
(Node *) list_make1(makeBoolConst(false,
1109+
false)),
1110+
NULL);
1111+
1112+
/* These lists must be nonempty to make a valid ModifyTable node */
1113+
subplans = list_make1(subplan);
1114+
resultRelations = list_make1_int(parse->resultRelation);
1115+
if (parse->withCheckOptions)
1116+
withCheckOptionLists = list_make1(parse->withCheckOptions);
1117+
if (parse->returningList)
1118+
returningLists = list_make1(parse->returningList);
1119+
}
1120+
else
1121+
{
1122+
/*
1123+
* Put back the final adjusted rtable into the master copy of the
1124+
* Query. (We mustn't do this if we found no non-excluded children,
1125+
* since we never saved an adjusted rtable at all.)
1126+
*/
1127+
parse->rtable = final_rtable;
1128+
root->simple_rel_array_size = save_rel_array_size;
1129+
root->simple_rel_array = save_rel_array;
1130+
}
11131131

11141132
/*
11151133
* If there was a FOR [KEY] UPDATE/SHARE clause, the LockRows node will

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

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,45 @@ CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a);
539539
INSERT INTO z VALUES (NULL, 'text'); -- should fail
540540
ERROR: null value in column "aa" violates not-null constraint
541541
DETAIL: Failing row contains (null, text).
542+
-- Check inherited UPDATE with all children excluded
543+
create table some_tab (a int, b int);
544+
create table some_tab_child () inherits (some_tab);
545+
insert into some_tab_child values(1,2);
546+
explain (verbose, costs off)
547+
update some_tab set a = a + 1 where false;
548+
QUERY PLAN
549+
-------------------------------------------------------------
550+
Update on public.some_tab
551+
-> Result
552+
Output: (some_tab.a + 1), some_tab.b, some_tab.ctid
553+
One-Time Filter: false
554+
(4 rows)
555+
556+
update some_tab set a = a + 1 where false;
557+
explain (verbose, costs off)
558+
update some_tab set a = a + 1 where false returning b, a;
559+
QUERY PLAN
560+
-------------------------------------------------------------
561+
Update on public.some_tab
562+
Output: some_tab.b, some_tab.a
563+
-> Result
564+
Output: (some_tab.a + 1), some_tab.b, some_tab.ctid
565+
One-Time Filter: false
566+
(5 rows)
567+
568+
update some_tab set a = a + 1 where false returning b, a;
569+
b | a
570+
---+---
571+
(0 rows)
572+
573+
table some_tab;
574+
a | b
575+
---+---
576+
1 | 2
577+
(1 row)
578+
579+
drop table some_tab cascade;
580+
NOTICE: drop cascades to table some_tab_child
542581
-- Check UPDATE with inherited target and an inherited source table
543582
create temp table foo(f1 int, f2 int);
544583
create temp table foo2(f3 int) inherits (foo);

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

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1757,3 +1757,37 @@ select * from self_ref_trigger;
17571757
drop table self_ref_trigger;
17581758
drop function self_ref_trigger_ins_func();
17591759
drop function self_ref_trigger_del_func();
1760+
--
1761+
-- Check that statement triggers work correctly even with all children excluded
1762+
--
1763+
create table stmt_trig_on_empty_upd (a int);
1764+
create table stmt_trig_on_empty_upd1 () inherits (stmt_trig_on_empty_upd);
1765+
create function update_stmt_notice() returns trigger as $$
1766+
begin
1767+
raise notice 'updating %', TG_TABLE_NAME;
1768+
return null;
1769+
end;
1770+
$$ language plpgsql;
1771+
create trigger before_stmt_trigger
1772+
before update on stmt_trig_on_empty_upd
1773+
execute procedure update_stmt_notice();
1774+
create trigger before_stmt_trigger
1775+
before update on stmt_trig_on_empty_upd1
1776+
execute procedure update_stmt_notice();
1777+
-- inherited no-op update
1778+
update stmt_trig_on_empty_upd set a = a where false returning a+1 as aa;
1779+
NOTICE: updating stmt_trig_on_empty_upd
1780+
aa
1781+
----
1782+
(0 rows)
1783+
1784+
-- simple no-op update
1785+
update stmt_trig_on_empty_upd1 set a = a where false returning a+1 as aa;
1786+
NOTICE: updating stmt_trig_on_empty_upd1
1787+
aa
1788+
----
1789+
(0 rows)
1790+
1791+
drop table stmt_trig_on_empty_upd cascade;
1792+
NOTICE: drop cascades to table stmt_trig_on_empty_upd1
1793+
drop function update_stmt_notice();

‎src/test/regress/sql/inherit.sql

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,21 @@ SELECT relname, d.* FROM ONLY d, pg_class where d.tableoid = pg_class.oid;
9797
CREATE TEMP TABLE z (b TEXT, PRIMARY KEY(aa, b)) inherits (a);
9898
INSERT INTO z VALUES (NULL, 'text'); -- should fail
9999

100+
-- Check inherited UPDATE with all children excluded
101+
create table some_tab (a int, b int);
102+
create table some_tab_child () inherits (some_tab);
103+
insert into some_tab_child values(1,2);
104+
105+
explain (verbose, costs off)
106+
update some_tab set a = a + 1 where false;
107+
update some_tab set a = a + 1 where false;
108+
explain (verbose, costs off)
109+
update some_tab set a = a + 1 where false returning b, a;
110+
update some_tab set a = a + 1 where false returning b, a;
111+
table some_tab;
112+
113+
drop table some_tab cascade;
114+
100115
-- Check UPDATE with inherited target and an inherited source table
101116
create temp table foo(f1 int, f2 int);
102117
create temp table foo2(f3 int) inherits (foo);

‎src/test/regress/sql/triggers.sql

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1189,3 +1189,30 @@ select * from self_ref_trigger;
11891189
drop table self_ref_trigger;
11901190
drop function self_ref_trigger_ins_func();
11911191
drop function self_ref_trigger_del_func();
1192+
1193+
--
1194+
-- Check that statement triggers work correctly even with all children excluded
1195+
--
1196+
1197+
create table stmt_trig_on_empty_upd (a int);
1198+
create table stmt_trig_on_empty_upd1 () inherits (stmt_trig_on_empty_upd);
1199+
create function update_stmt_notice() returns trigger as $$
1200+
begin
1201+
raise notice 'updating %', TG_TABLE_NAME;
1202+
return null;
1203+
end;
1204+
$$ language plpgsql;
1205+
create trigger before_stmt_trigger
1206+
before update on stmt_trig_on_empty_upd
1207+
execute procedure update_stmt_notice();
1208+
create trigger before_stmt_trigger
1209+
before update on stmt_trig_on_empty_upd1
1210+
execute procedure update_stmt_notice();
1211+
1212+
-- inherited no-op update
1213+
update stmt_trig_on_empty_upd set a = a where false returning a+1 as aa;
1214+
-- simple no-op update
1215+
update stmt_trig_on_empty_upd1 set a = a where false returning a+1 as aa;
1216+
1217+
drop table stmt_trig_on_empty_upd cascade;
1218+
drop function update_stmt_notice();

0 commit comments

Comments
 (0)