Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit06ba530

Browse files
committed
Fix creation of resjunk tlist entries for inherited mixed UPDATE/DELETE.
rewriteTargetListUD's processing is dependent on the relkind of the query'starget table. That was fine at the time it was made to act that way, evenfor queries on inheritance trees, because all tables in an inheritance treewould necessarily be plain tables. However, the 9.5 feature additionallowing some members of an inheritance tree to be foreign tables broke theassumption that rewriteTargetListUD's output tlist could be applied to allchild tables with nothing more than column-number mapping. This led tovisible failures if foreign child tables had row-level triggers, and wouldalso break in cases where child tables belonged to FDWs that used methodsother than CTID for row identification.To fix, delay running rewriteTargetListUD until after the planner hasexpanded inheritance, so that it is applied separately to the (alreadymapped) tlist for each child table. We can conveniently call it frompreprocess_targetlist. Refactor associated code slightly to avoid theneed to heap_open the target relation multiple times duringpreprocess_targetlist. (The APIs remain a bit ugly, particularly aroundthe point of which steps scribble on parse->targetList and which don't.But avoiding such scribbling would require a change in FDW callback APIs,which is more pain than it's worth.)Also fix ExecModifyTable to ensure that "tupleid" is reset to NULL whenwe transition from rows providing a CTID to rows that don't. (That'sreally an independent bug, but it manifests in much the same cases.)Add a regression test checking one manifestation of this problem, whichwas that row-level triggers on a foreign child table did not work right.Back-patch to 9.5 where the problem was introduced.Etsuro Fujita, reviewed by Ildus Kurbangaliev and Ashutosh BapatDiscussion:https://postgr.es/m/20170514150525.0346ba72@postgrespro.ru
1 parent510cc2e commit06ba530

File tree

10 files changed

+185
-101
lines changed

10 files changed

+185
-101
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5777,6 +5777,63 @@ update bar set f2 = f2 + 100 returning *;
57775777
7 | 277
57785778
(6 rows)
57795779

5780+
-- Test that UPDATE/DELETE with inherited target works with row-level triggers
5781+
CREATE TRIGGER trig_row_before
5782+
BEFORE UPDATE OR DELETE ON bar2
5783+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
5784+
CREATE TRIGGER trig_row_after
5785+
AFTER UPDATE OR DELETE ON bar2
5786+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
5787+
explain (verbose, costs off)
5788+
update bar set f2 = f2 + 100;
5789+
QUERY PLAN
5790+
--------------------------------------------------------------------------------------
5791+
Update on public.bar
5792+
Update on public.bar
5793+
Foreign Update on public.bar2
5794+
Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3
5795+
-> Seq Scan on public.bar
5796+
Output: bar.f1, (bar.f2 + 100), bar.ctid
5797+
-> Foreign Scan on public.bar2
5798+
Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.*
5799+
Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
5800+
(9 rows)
5801+
5802+
update bar set f2 = f2 + 100;
5803+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
5804+
NOTICE: OLD: (3,333,33),NEW: (3,433,33)
5805+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
5806+
NOTICE: OLD: (4,344,44),NEW: (4,444,44)
5807+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
5808+
NOTICE: OLD: (7,277,77),NEW: (7,377,77)
5809+
NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
5810+
NOTICE: OLD: (3,333,33),NEW: (3,433,33)
5811+
NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
5812+
NOTICE: OLD: (4,344,44),NEW: (4,444,44)
5813+
NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
5814+
NOTICE: OLD: (7,277,77),NEW: (7,377,77)
5815+
explain (verbose, costs off)
5816+
delete from bar where f2 < 400;
5817+
QUERY PLAN
5818+
---------------------------------------------------------------------------------------------
5819+
Delete on public.bar
5820+
Delete on public.bar
5821+
Foreign Delete on public.bar2
5822+
Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3
5823+
-> Seq Scan on public.bar
5824+
Output: bar.ctid
5825+
Filter: (bar.f2 < 400)
5826+
-> Foreign Scan on public.bar2
5827+
Output: bar2.ctid, bar2.*
5828+
Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE
5829+
(10 rows)
5830+
5831+
delete from bar where f2 < 400;
5832+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2
5833+
NOTICE: OLD: (7,377,77)
5834+
NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2
5835+
NOTICE: OLD: (7,377,77)
5836+
-- cleanup
57805837
drop table foo cascade;
57815838
NOTICE: drop cascades to foreign table foo2
57825839
drop table bar cascade;

‎contrib/postgres_fdw/sql/postgres_fdw.sql

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1318,6 +1318,24 @@ explain (verbose, costs off)
13181318
update barset f2= f2+100 returning*;
13191319
update barset f2= f2+100 returning*;
13201320

1321+
-- Test that UPDATE/DELETE with inherited target works with row-level triggers
1322+
CREATETRIGGERtrig_row_before
1323+
BEFOREUPDATEORDELETEON bar2
1324+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
1325+
1326+
CREATETRIGGERtrig_row_after
1327+
AFTERUPDATEORDELETEON bar2
1328+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
1329+
1330+
explain (verbose, costs off)
1331+
update barset f2= f2+100;
1332+
update barset f2= f2+100;
1333+
1334+
explain (verbose, costs off)
1335+
deletefrom barwhere f2<400;
1336+
deletefrom barwhere f2<400;
1337+
1338+
-- cleanup
13211339
droptable foo cascade;
13221340
droptable bar cascade;
13231341
droptable loct1;

‎doc/src/sgml/fdwhandler.sgml

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -429,11 +429,14 @@ AddForeignUpdateTargets (Query *parsetree,
429429
<literal>wholerow</literal>, or
430430
<literal>wholerow<replaceable>N</></literal>, as the core system can
431431
generate junk columns of these names.
432+
If the extra expressions are more complex than simple Vars, they
433+
must be run through <function>eval_const_expressions</function>
434+
before adding them to the targetlist.
432435
</para>
433436

434437
<para>
435-
Thisfunction is calledin the rewriter, not the planner, so the
436-
informationavailable is a bit different from that available tothe
438+
Although thisfunction is calledduring planning, the
439+
informationprovided is a bit different from that available toother
437440
planning routines.
438441
<literal>parsetree</> is the parse tree for the <command>UPDATE</> or
439442
<command>DELETE</> command, while <literal>target_rte</> and

‎doc/src/sgml/rules.sgml

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -167,12 +167,12 @@
167167

168168
<para>
169169
<command>DELETE</command> commands don't need a normal target list
170-
because they don't produce any result. Instead, therule system
170+
because they don't produce any result. Instead, theplanner
171171
adds a special <acronym>CTID</> entry to the empty target list,
172172
to allow the executor to find the row to be deleted.
173173
(<acronym>CTID</> is added when the result relation is an ordinary
174-
table. If it is a view, a whole-row variable is added instead,
175-
as described in <xref linkend="rules-views-update">.)
174+
table. If it is a view, a whole-row variable is added instead, by
175+
the rule system,as described in <xref linkend="rules-views-update">.)
176176
</para>
177177

178178
<para>
@@ -194,7 +194,7 @@
194194
column = expression</literal> part of the command. The planner will
195195
handle missing columns by inserting expressions that copy the values
196196
from the old row into the new one. Just as for <command>DELETE</>,
197-
the rule system addsa <acronym>CTID</> or whole-row variable so that
197+
a <acronym>CTID</> or whole-row variable is added so that
198198
the executor can identify the old row to be updated.
199199
</para>
200200

‎src/backend/executor/nodeModifyTable.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1324,7 +1324,7 @@ ExecModifyTable(ModifyTableState *node)
13241324
JunkFilter*junkfilter;
13251325
TupleTableSlot*slot;
13261326
TupleTableSlot*planSlot;
1327-
ItemPointertupleid=NULL;
1327+
ItemPointertupleid;
13281328
ItemPointerDatatuple_ctid;
13291329
HeapTupleDataoldtupdata;
13301330
HeapTupleoldtuple;
@@ -1432,6 +1432,7 @@ ExecModifyTable(ModifyTableState *node)
14321432
EvalPlanQualSetSlot(&node->mt_epqstate,planSlot);
14331433
slot=planSlot;
14341434

1435+
tupleid=NULL;
14351436
oldtuple=NULL;
14361437
if (junkfilter!=NULL)
14371438
{

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

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,7 +1399,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
13991399
doubletuple_fraction)
14001400
{
14011401
Query*parse=root->parse;
1402-
List*tlist=parse->targetList;
1402+
List*tlist;
14031403
int64offset_est=0;
14041404
int64count_est=0;
14051405
doublelimit_tuples=-1.0;
@@ -1596,13 +1596,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
15961596
}
15971597

15981598
/* Preprocess targetlist */
1599-
tlist=preprocess_targetlist(root,tlist);
1600-
1601-
if (parse->onConflict)
1602-
parse->onConflict->onConflictSet=
1603-
preprocess_onconflict_targetlist(parse->onConflict->onConflictSet,
1604-
parse->resultRelation,
1605-
parse->rtable);
1599+
tlist=preprocess_targetlist(root);
16061600

16071601
/*
16081602
* Expand any rangetable entries that have security barrier quals.

‎src/backend/optimizer/prep/preptlist.c

Lines changed: 62 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -4,20 +4,22 @@
44
* Routines to preprocess the parse tree target list
55
*
66
* For INSERT and UPDATE queries, the targetlist must contain an entry for
7-
* each attribute of the target relation in the correct order. For all query
7+
* each attribute of the target relation in the correct order. For UPDATE and
8+
* DELETE queries, it must also contain junk tlist entries needed to allow the
9+
* executor to identify the rows to be updated or deleted. For all query
810
* types, we may need to add junk tlist entries for Vars used in the RETURNING
911
* list and row ID information needed for SELECT FOR UPDATE locking and/or
1012
* EvalPlanQual checking.
1113
*
12-
* Therewriter's rewriteTargetListIU and rewriteTargetListUD routines
13-
*also do preprocessing of the targetlist. The division of labor between
14-
*here and there ispartially historical, but it's not entirely arbitrary.
15-
*In particular,consider an UPDATE across an inheritance tree. Whatthe
16-
*rewriterdoes need be done only once (because it depends only on the
17-
*properties ofthe parent relation). What's done here has to be done over
18-
*again for eachchild relation, because it depends on thecolumn list of
19-
*the child, which mighthave more columns and/or a different column order
20-
* than the parent.
14+
* Thequery rewrite phase also does preprocessing of the targetlist (see
15+
*rewriteTargetListIU). The division of labor between here and there is
16+
* partially historical, but it's not entirely arbitrary. In particular,
17+
* consider an UPDATE across an inheritance tree. WhatrewriteTargetListIU
18+
* does need be done only once (because it depends only on the properties of
19+
* the parent relation). What's done here has to be done over again for each
20+
* child relation, because it depends on theproperties of the child, which
21+
*might be of a different relation type, orhave more columns and/or a
22+
*different column orderthan the parent.
2123
*
2224
* The fact that rewriteTargetListIU sorts non-resjunk tlist entries by column
2325
* position, which expand_targetlist depends on, violates the above comment
@@ -47,48 +49,74 @@
4749
#include"optimizer/var.h"
4850
#include"parser/parsetree.h"
4951
#include"parser/parse_coerce.h"
52+
#include"rewrite/rewriteHandler.h"
5053
#include"utils/rel.h"
5154

5255

5356
staticList*expand_targetlist(List*tlist,intcommand_type,
54-
Indexresult_relation,List*range_table);
57+
Indexresult_relation,Relationrel);
5558

5659

5760
/*
5861
* preprocess_targetlist
5962
* Driver for preprocessing the parse tree targetlist.
6063
*
6164
* Returns the new targetlist.
65+
*
66+
* As a side effect, if there's an ON CONFLICT UPDATE clause, its targetlist
67+
* is also preprocessed (and updated in-place).
6268
*/
6369
List*
64-
preprocess_targetlist(PlannerInfo*root,List*tlist)
70+
preprocess_targetlist(PlannerInfo*root)
6571
{
6672
Query*parse=root->parse;
6773
intresult_relation=parse->resultRelation;
6874
List*range_table=parse->rtable;
6975
CmdTypecommand_type=parse->commandType;
76+
RangeTblEntry*target_rte=NULL;
77+
Relationtarget_relation=NULL;
78+
List*tlist;
7079
ListCell*lc;
7180

7281
/*
73-
* Sanity check: if there is a result relation, it'd better be a real
74-
* relation not a subquery. Else parser or rewriter messed up.
82+
* If there is a result relation, open it so we can look for missing
83+
* columns and so on. We assume that previous code already acquired at
84+
* least AccessShareLock on the relation, so we need no lock here.
7585
*/
7686
if (result_relation)
7787
{
78-
RangeTblEntry*rte=rt_fetch(result_relation,range_table);
88+
target_rte=rt_fetch(result_relation,range_table);
89+
90+
/*
91+
* Sanity check: it'd better be a real relation not, say, a subquery.
92+
* Else parser or rewriter messed up.
93+
*/
94+
if (target_rte->rtekind!=RTE_RELATION)
95+
elog(ERROR,"result relation must be a regular relation");
7996

80-
if (rte->subquery!=NULL||rte->relid==InvalidOid)
81-
elog(ERROR,"subquery cannot be result relation");
97+
target_relation=heap_open(target_rte->relid,NoLock);
8298
}
99+
else
100+
Assert(command_type==CMD_SELECT);
101+
102+
/*
103+
* For UPDATE/DELETE, add any junk column(s) needed to allow the executor
104+
* to identify the rows to be updated or deleted. Note that this step
105+
* scribbles on parse->targetList, which is not very desirable, but we
106+
* keep it that way to avoid changing APIs used by FDWs.
107+
*/
108+
if (command_type==CMD_UPDATE||command_type==CMD_DELETE)
109+
rewriteTargetListUD(parse,target_rte,target_relation);
83110

84111
/*
85112
* for heap_form_tuple to work, the targetlist must match the exact order
86113
* of the attributes. We also need to fill in any missing attributes. -ay
87114
* 10/94
88115
*/
116+
tlist=parse->targetList;
89117
if (command_type==CMD_INSERT||command_type==CMD_UPDATE)
90118
tlist=expand_targetlist(tlist,command_type,
91-
result_relation,range_table);
119+
result_relation,target_relation);
92120

93121
/*
94122
* Add necessary junk columns for rowmarked rels. These values are needed
@@ -193,19 +221,21 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
193221
list_free(vars);
194222
}
195223

196-
returntlist;
197-
}
224+
/*
225+
* If there's an ON CONFLICT UPDATE clause, preprocess its targetlist too
226+
* while we have the relation open.
227+
*/
228+
if (parse->onConflict)
229+
parse->onConflict->onConflictSet=
230+
expand_targetlist(parse->onConflict->onConflictSet,
231+
CMD_UPDATE,
232+
result_relation,
233+
target_relation);
198234

199-
/*
200-
* preprocess_onconflict_targetlist
201-
* Process ON CONFLICT SET targetlist.
202-
*
203-
* Returns the new targetlist.
204-
*/
205-
List*
206-
preprocess_onconflict_targetlist(List*tlist,intresult_relation,List*range_table)
207-
{
208-
returnexpand_targetlist(tlist,CMD_UPDATE,result_relation,range_table);
235+
if (target_relation)
236+
heap_close(target_relation,NoLock);
237+
238+
returntlist;
209239
}
210240

211241

@@ -223,11 +253,10 @@ preprocess_onconflict_targetlist(List *tlist, int result_relation, List *range_t
223253
*/
224254
staticList*
225255
expand_targetlist(List*tlist,intcommand_type,
226-
Indexresult_relation,List*range_table)
256+
Indexresult_relation,Relationrel)
227257
{
228258
List*new_tlist=NIL;
229259
ListCell*tlist_item;
230-
Relationrel;
231260
intattrno,
232261
numattrs;
233262

@@ -238,12 +267,8 @@ expand_targetlist(List *tlist, int command_type,
238267
* order; but we have to insert TLEs for any missing attributes.
239268
*
240269
* Scan the tuple description in the relation's relcache entry to make
241-
* sure we have all the user attributes in the right order. We assume
242-
* that the rewriter already acquired at least AccessShareLock on the
243-
* relation, so we need no lock here.
270+
* sure we have all the user attributes in the right order.
244271
*/
245-
rel=heap_open(getrelid(result_relation,range_table),NoLock);
246-
247272
numattrs=RelationGetNumberOfAttributes(rel);
248273

249274
for (attrno=1;attrno <=numattrs;attrno++)
@@ -386,8 +411,6 @@ expand_targetlist(List *tlist, int command_type,
386411
tlist_item=lnext(tlist_item);
387412
}
388413

389-
heap_close(rel,NoLock);
390-
391414
returnnew_tlist;
392415
}
393416

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp