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

Commit9a785ad

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 parent1174690 commit9a785ad

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
@@ -7022,6 +7022,63 @@ update bar set f2 = f2 + 100 returning *;
70227022
7 | 277
70237023
(6 rows)
70247024

7025+
-- Test that UPDATE/DELETE with inherited target works with row-level triggers
7026+
CREATE TRIGGER trig_row_before
7027+
BEFORE UPDATE OR DELETE ON bar2
7028+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
7029+
CREATE TRIGGER trig_row_after
7030+
AFTER UPDATE OR DELETE ON bar2
7031+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
7032+
explain (verbose, costs off)
7033+
update bar set f2 = f2 + 100;
7034+
QUERY PLAN
7035+
--------------------------------------------------------------------------------------
7036+
Update on public.bar
7037+
Update on public.bar
7038+
Foreign Update on public.bar2
7039+
Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3
7040+
-> Seq Scan on public.bar
7041+
Output: bar.f1, (bar.f2 + 100), bar.ctid
7042+
-> Foreign Scan on public.bar2
7043+
Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.*
7044+
Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE
7045+
(9 rows)
7046+
7047+
update bar set f2 = f2 + 100;
7048+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
7049+
NOTICE: OLD: (3,333,33),NEW: (3,433,33)
7050+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
7051+
NOTICE: OLD: (4,344,44),NEW: (4,444,44)
7052+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2
7053+
NOTICE: OLD: (7,277,77),NEW: (7,377,77)
7054+
NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
7055+
NOTICE: OLD: (3,333,33),NEW: (3,433,33)
7056+
NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
7057+
NOTICE: OLD: (4,344,44),NEW: (4,444,44)
7058+
NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2
7059+
NOTICE: OLD: (7,277,77),NEW: (7,377,77)
7060+
explain (verbose, costs off)
7061+
delete from bar where f2 < 400;
7062+
QUERY PLAN
7063+
---------------------------------------------------------------------------------------------
7064+
Delete on public.bar
7065+
Delete on public.bar
7066+
Foreign Delete on public.bar2
7067+
Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3
7068+
-> Seq Scan on public.bar
7069+
Output: bar.ctid
7070+
Filter: (bar.f2 < 400)
7071+
-> Foreign Scan on public.bar2
7072+
Output: bar2.ctid, bar2.*
7073+
Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 WHERE ((f2 < 400)) FOR UPDATE
7074+
(10 rows)
7075+
7076+
delete from bar where f2 < 400;
7077+
NOTICE: trig_row_before(23, skidoo) BEFORE ROW DELETE ON bar2
7078+
NOTICE: OLD: (7,377,77)
7079+
NOTICE: trig_row_after(23, skidoo) AFTER ROW DELETE ON bar2
7080+
NOTICE: OLD: (7,377,77)
7081+
-- cleanup
70257082
drop table foo cascade;
70267083
NOTICE: drop cascades to foreign table foo2
70277084
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
@@ -1656,6 +1656,24 @@ explain (verbose, costs off)
16561656
update barset f2= f2+100 returning*;
16571657
update barset f2= f2+100 returning*;
16581658

1659+
-- Test that UPDATE/DELETE with inherited target works with row-level triggers
1660+
CREATETRIGGERtrig_row_before
1661+
BEFOREUPDATEORDELETEON bar2
1662+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
1663+
1664+
CREATETRIGGERtrig_row_after
1665+
AFTERUPDATEORDELETEON bar2
1666+
FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo');
1667+
1668+
explain (verbose, costs off)
1669+
update barset f2= f2+100;
1670+
update barset f2= f2+100;
1671+
1672+
explain (verbose, costs off)
1673+
deletefrom barwhere f2<400;
1674+
deletefrom barwhere f2<400;
1675+
1676+
-- cleanup
16591677
droptable foo cascade;
16601678
droptable bar cascade;
16611679
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</replaceable></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</literal> is the parse tree for the <command>UPDATE</command> or
439442
<command>DELETE</command> command, while <literal>target_rte</literal> 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</acronym> entry to the empty target list,
172172
to allow the executor to find the row to be deleted.
173173
(<acronym>CTID</acronym> 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</command>,
197-
the rule system addsa <acronym>CTID</acronym> or whole-row variable so that
197+
a <acronym>CTID</acronym> 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
@@ -1576,7 +1576,7 @@ ExecModifyTable(PlanState *pstate)
15761576
JunkFilter*junkfilter;
15771577
TupleTableSlot*slot;
15781578
TupleTableSlot*planSlot;
1579-
ItemPointertupleid=NULL;
1579+
ItemPointertupleid;
15801580
ItemPointerDatatuple_ctid;
15811581
HeapTupleDataoldtupdata;
15821582
HeapTupleoldtuple;
@@ -1699,6 +1699,7 @@ ExecModifyTable(PlanState *pstate)
16991699
EvalPlanQualSetSlot(&node->mt_epqstate,planSlot);
17001700
slot=planSlot;
17011701

1702+
tupleid=NULL;
17021703
oldtuple=NULL;
17031704
if (junkfilter!=NULL)
17041705
{

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

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1555,7 +1555,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
15551555
doubletuple_fraction)
15561556
{
15571557
Query*parse=root->parse;
1558-
List*tlist=parse->targetList;
1558+
List*tlist;
15591559
int64offset_est=0;
15601560
int64count_est=0;
15611561
doublelimit_tuples=-1.0;
@@ -1685,13 +1685,7 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
16851685
}
16861686

16871687
/* Preprocess targetlist */
1688-
tlist=preprocess_targetlist(root,tlist);
1689-
1690-
if (parse->onConflict)
1691-
parse->onConflict->onConflictSet=
1692-
preprocess_onconflict_targetlist(parse->onConflict->onConflictSet,
1693-
parse->resultRelation,
1694-
parse->rtable);
1688+
tlist=preprocess_targetlist(root);
16951689

16961690
/*
16971691
* We are now done hacking up the query's targetlist. Most of the

‎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