8000 Fix state reversal after partition tuple routing · papercuptech/postgres@6666ee4 · GitHub
[go: up one dir, main page]
Skip to content

Commit 6666ee4

Browse files
alvherrealvherre
committed
Fix state reversal after partition tuple routing
We make some changes to ModifyTableState and the EState it uses whenever we route tuples to partitions; but we weren't restoring properly in all cases, possibly causing crashes when partitions with different tuple descriptors are targeted by tuples inserted in the same command. Refactor some code, creating ExecPrepareTupleRouting, to encapsulate the needed state changing logic, and have it invoked one level above its current place (ie. put it in ExecModifyTable instead of ExecInsert); this makes it all more readable. Add a test case to exercise this. We don't support having views as partitions; and since only views can have INSTEAD OF triggers, there is no point in testing for INSTEAD OF when processing insertions into a partitioned table. Remove code that appears to support this (but which is actually never relevant.) In passing, fix location of some very confusing comments in ModifyTableState. Reported-by: Amit Langote Author: Etsuro Fujita, Amit Langote Discussion: https://postgr/es/m/0473bf5c-57b1-f1f7-3d58-455c2230bc5f@lab.ntt.co.jp
1 parent c596fad commit 6666ee4

File tree

5 files changed

+188
-123
lines changed

5 files changed

+188
-123
lines changed

‎src/backend/commands/copy.c

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2633,13 +2633,12 @@ CopyFrom(CopyState cstate)
26332633
if (cstate->transition_capture != NULL)
26342634
{
26352635
if (resultRelInfo->ri_TrigDesc &&
2636-
(resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
2637-
resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
2636+
resultRelInfo->ri_TrigDesc->trig_insert_before_row)
26382637
{
26392638
/*
2640-
* If there are any BEFORE or INSTEAD triggers on the
2641-
* partition, we'll have to be ready to convert their
2642-
* result back to tuplestore format.
2639+
* If there are any BEFORE triggers on the partition,
2640+
* we'll have to be ready to convert their result back to
2641+
* tuplestore format.
26432642
*/
26442643
cstate->transition_capture->tcs_original_insert_tuple = NULL;
26452644
cstate->transition_capture->tcs_map =
@@ -2768,12 +2767,13 @@ CopyFrom(CopyState cstate)
27682767
* tuples inserted by an INSERT command.
27692768
*/
27702769
processed++;
2770+
}
27712771

2772-
if (saved_resultRelInfo)
2773-
{
2774-
resultRelInfo = saved_resultRelInfo;
2775-
estate->es_result_relation_info = resultRelInfo;
2776-
}
2772+
/* Restore the saved ResultRelInfo */
2773+
if (saved_resultRelInfo)
2774+
{
2775+
resultRelInfo = saved_resultRelInfo;
2776+
estate->es_result_relation_info = resultRelInfo;
27772777
}
27782778
}
27792779

‎src/backend/executor/nodeModifyTable.c

Lines changed: 121 additions & 109 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,11 @@ static bool ExecOnConflictUpdate(ModifyTableState *mtstate,
6262
EState *estate,
6363
bool canSetTag,
6464
TupleTableSlot **returning);
65+
static TupleTableSlot *ExecPrepareTupleRouting(ModifyTableState *mtstate,
66+
EState *estate,
67+
PartitionTupleRouting *proute,
68+
ResultRelInfo *targetRelInfo,
69+
TupleTableSlot *slot);
6570
static ResultRelInfo *getTargetResultRelInfo(ModifyTableState *node);
6671
static void ExecSetupChildParentMapForTcs(ModifyTableState *mtstate);
6772
static void ExecSetupChildParentMapForSubplan(ModifyTableState *mtstate);
@@ -265,7 +270,6 @@ ExecInsert(ModifyTableState *mtstate,
265270
{
266271
HeapTuple tuple;
267272
ResultRelInfo *resultRelInfo;
268-
ResultRelInfo *saved_resultRelInfo = NULL;
269273
Relation resultRelationDesc;
270274
Oid newId;
271275
List *recheckIndexes = NIL;
@@ -282,100 +286,6 @@ ExecInsert(ModifyTableState *mtstate,
282286
* get information on the (current) result relation
283287
*/
284288
resultRelInfo = estate->es_result_relation_info;
285-
286-
/* Determine the partition to heap_insert the tuple into */
287-
if (mtstate->mt_partition_tuple_routing)
288-
{
289-
int leaf_part_index;
290-
PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing;
291-
292-
/*
293-
* Away we go ... If we end up not finding a partition after all,
294-
* ExecFindPartition() does not return and errors out instead.
295-
* Otherwise, the returned value is to be used as an index into arrays
296-
* proute->partitions[] and proute->partition_tupconv_maps[] that will
297-
* get us the ResultRelInfo and TupleConversionMap for the partition,
298-
* respectively.
299-
*/
300-
leaf_part_index = ExecFindPartition(resultRelInfo,
301-
proute->partition_dispatch_info,
302-
slot,
303-
estate);
304-
Assert(leaf_part_index >= 0 &&
305-
leaf_part_index < proute->num_partitions);
306-
307-
/*
308-
* Save the old ResultRelInfo and switch to the one corresponding to
309-
* the selected partition. (We might need to initialize it first.)
310-
*/
311-
saved_resultRelInfo = resultRelInfo;
312-
resultRelInfo = proute->partitions[leaf_part_index];
313-
if (resultRelInfo == NULL)
314-
{
315-
resultRelInfo = ExecInitPartitionInfo(mtstate,
316-
saved_resultRelInfo,
317-
proute, estate,
318-
leaf_part_index);
319-
Assert(resultRelInfo != NULL);
320-
}
321-
322-
/* We do not yet have a way to insert into a foreign partition */
323-
if (resultRelInfo->ri_FdwRoutine)
324-
ereport(ERROR,
325-
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
326-
errmsg("cannot route inserted tuples to a foreign table")));
327-
328-
/* For ExecInsertIndexTuples() to work on the partition's indexes */
329-
estate->es_result_relation_info = resultRelInfo;
330-
331-
/*
332-
* If we're capturing transition tuples, we might need to convert from
333-
* the partition rowtype to parent rowtype.
334-
*/
335-
if (mtstate->mt_transition_capture != NULL)
336-
{
337-
if (resultRelInfo->ri_TrigDesc &&
338-
(resultRelInfo->ri_TrigDesc->trig_insert_before_row ||
339-
resultRelInfo->ri_TrigDesc->trig_insert_instead_row))
340-
{
341-
/*
342-
* If there are any BEFORE or INSTEAD triggers on the
343-
* partition, we'll have to be ready to convert their result
344-
* back to tuplestore format.
345-
*/
346-
mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
347-
348-
mtstate->mt_transition_capture->tcs_map =
349-
TupConvMapForLeaf(proute, saved_resultRelInfo,
350-
leaf_part_index);
351-
}
352-
else
353-
{
354-
/*
355-
* Otherwise, just remember the original unconverted tuple, to
356-
* avoid a needless round trip conversion.
357-
*/
358-
mtstate->mt_transition_capture->tcs_original_insert_tuple = tuple;
359-
mtstate->mt_transition_capture->tcs_map = NULL;
360-
}
361-
}
362-
if (mtstate->mt_oc_transition_capture != NULL)
363-
{
364-
mtstate->mt_oc_transition_capture->tcs_map =
365-
TupConvMapForLeaf(proute, saved_resultRelInfo,
366-
leaf_part_index);
367-
}
368-
369-
/*
370-
* We might need to convert from the parent rowtype to the partition
371-
* rowtype.
372-
*/
373-
tuple = ConvertPartitionTupleSlot(proute->parent_child_tupconv_maps[leaf_part_index],
374-
tuple,
375-
proute->partition_tuple_slot,
376-
&slot);
377-
}
378-
379289
resultRelationDesc = resultRelInfo->ri_RelationDesc;
380290

381291
/*
@@ -495,7 +405,7 @@ ExecInsert(ModifyTableState *mtstate,
495405
* No need though if the tuple has been routed, and a BR trigger
496406
* doesn't exist.
497407
*/
498-
if (saved_resultRelInfo != NULL &&
408+
if (resultRelInfo->ri_PartitionRoot != NULL &&
499409
!(resultRelInfo->ri_TrigDesc &&
500410
resultRelInfo->ri_TrigDesc->trig_insert_before_row))
501411
check_partition_constr = false;
@@ -686,9 +596,6 @@ ExecInsert(ModifyTableState *mtstate,
686596
if (resultRelInfo->ri_projectReturning)
687597
result = ExecProcessReturning(resultRelInfo, slot, planSlot);
688598

689-
if (saved_resultRelInfo)
690-
estate->es_result_relation_info = saved_resultRelInfo;
691-
692599
return result;
693600
}
694601

@@ -1209,27 +1116,22 @@ lreplace:;
12091116
proute->root_tuple_slot,
12101117
&slot);
12111118

1212-
1213-
/*
1214-
* For ExecInsert(), make it look like we are inserting into the
1215-
* root.
1216-
*/
1119+
/* Prepare for tuple routing */
12171120
Assert(mtstate->rootResultRelInfo != NULL);
1218-
estate->es_result_relation_info = mtstate->rootResultRelInfo;
1121+
slot = ExecPrepareTupleRouting(mtstate, estate, proute,
1122+
mtstate->rootResultRelInfo, slot);
12191123

12201124
ret_slot = ExecInsert(mtstate, slot, planSlot, NULL,
12211125
ONCONFLICT_NONE, estate, canSetTag);
12221126

1223-
/*
1224-
* Revert back the active result relation and the active
1225-
* transition capture map that we changed above.
1226-
*/
1127+
/* Revert ExecPrepareTupleRouting's node change. */
12271128
estate->es_result_relation_info = resultRelInfo;
12281129
if (mtstate->mt_transition_capture)
12291130
{
12301131
mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
12311132
mtstate->mt_transition_capture->tcs_map = saved_tcs_map;
12321133
}
1134+
12331135
return ret_slot;
12341136
}
12351137

@@ -1710,6 +1612,108 @@ ExecSetupTransitionCaptureState(ModifyTableState *mtstate, EState *estate)
17101612
}
17111613
}
17121614

1615+
/*
1616+
* ExecPrepareTupleRouting --- prepare for routing one tuple
1617+
*
1618+
* Determine the partition in which the tuple in slot is to be inserted,
1619+
* and modify mtstate and estate to prepare for it.
1620+
*
1621+
* Caller must revert the estate changes after executing the insertion!
1622+
* In mtstate, transition capture changes may also need to be reverted.
1623+
*
1624+
* Returns a slot holding the tuple of the partition rowtype.
1625+
*/
1626+
static TupleTableSlot *
1627+
ExecPrepareTupleRouting(ModifyTableState *mtstate,
1628+
EState *estate,
1629+
PartitionTupleRouting *proute,
1630+
ResultRelInfo *targetRelInfo,
1631+
TupleTableSlot *slot)
1632+
{
1633+
int partidx;
1634+
ResultRelInfo *partrel;
1635+
HeapTuple tuple;
1636+
1637+
/*
1638+
* Determine the target partition. If ExecFindPartition does not find
1639+
* a partition after all, it doesn't return here; otherwise, the returned
1640+
* value is to be used as an index into the arrays for the ResultRelInfo
1641+
* and TupleConversionMap for the partition.
1642+
*/
1643+
partidx = ExecFindPartition(targetRelInfo,
1644+
proute->partition_dispatch_info,
1645+
slot,
1646+
estate);
1647+
Assert(partidx >= 0 && partidx < proute->num_partitions);
1648+
1649+
/*
1650+
* Get the ResultRelInfo corresponding to the selected partition; if not
1651+
* yet there, initialize it.
1652+
*/
1653+
partrel = proute->partitions[partidx];
1654+
if (partrel == NULL)
1655+
partrel = ExecInitPartitionInfo(mtstate, targetRelInfo,
1656+
proute, estate,
1657+
partidx);
1658+
1659+
/* We do not yet have a way to insert into a foreign partition */
1660+
if (partrel->ri_FdwRoutine)
1661+
ereport(ERROR,
1662+
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
1663+
errmsg("cannot route inserted tuples to a foreign table")));
1664+
1665+
/*
1666+
* Make it look like we are inserting into the partition.
1667+
*/
1668+
estate->es_result_relation_info = partrel;
1669+
1670+
/* Get the heap tuple out of the given slot. */
1671+
tuple = ExecMaterializeSlot(slot);
1672+
1673+
/*
1674+
* If we're capturing transition tuples, we might need to convert from the
1675+
* partition rowtype to parent rowtype.
1676+
*/
1677+
if (mtstate->mt_transition_capture != NULL)
1678+
{
1679+
if (partrel->ri_TrigDesc &&
1680+
partrel->ri_TrigDesc->trig_insert_before_row)
1681+
{
1682+
/*
1683+
* If there are any BEFORE triggers on the partition, we'll have
1684+
* to be ready to convert their result back to tuplestore format.
1685+
*/
1686+
mtstate->mt_transition_capture->tcs_original_insert_tuple = NULL;
1687+
mtstate->mt_transition_capture->tcs_map =
1688+
TupConvMapForLeaf(proute, targetRelInfo, partidx);
1689+
}
1690+
else
1691+
{
1692+
/*
1693+
* Otherwise, just remember the original unconverted tuple, to
1694+
* avoid a needless round trip conversion.
1695+
*/
1696+
mtstate->mt_transition_capture->tcs_original_insert_tuple = tuple;
1697+
mtstate->mt_transition_capture->tcs_map = NULL;
1698+
}
1699+
}
1700+
if (mtstate->mt_oc_transition_capture != NULL)
1701+
{
1702+
mtstate->mt_oc_transition_capture->tcs_map =
1703+
TupConvMapForLeaf(proute, targetRelInfo, partidx);
1704+
}
1705+
1706+
/*
1707+
* Convert the tuple, if necessary.
1708+
*/
1709+
ConvertPartitionTupleSlot(proute->parent_child_tupconv_maps[partidx],
1710+
tuple,
1711+
proute->partition_tuple_slot,
1712+
&slot);
1713+
1714+
return slot;
1715+
}
1716+
17131717
/*
17141718
* Initialize the child-to-root tuple conversion map array for UPDATE subplans.
17151719
*
@@ -1846,6 +1850,7 @@ static TupleTableSlot *
18461850
ExecModifyTable(PlanState *pstate)
18471851
{
18481852
ModifyTableState *node = castNode(ModifyTableState, pstate);
1853+
PartitionTupleRouting *proute = node->mt_partition_tuple_routing;
18491854
EState *estate = node->ps.state;
18501855
CmdType operation = node->operation;
18511856
ResultRelInfo *saved_resultRelInfo;
@@ -2051,9 +2056,16 @@ ExecModifyTable(PlanState *pstate)
20512056
switch (operation)
20522057
{
20532058
case CMD_INSERT:
2059+
/* Prepare for tuple routing if needed. */
2060+
if (proute)
2061+
slot = ExecPrepareTupleRouting(node, estate, proute,
2062+
resultRelInfo, slot);
20542063
slot = ExecInsert(node, slot, planSlot,
20552064
node->mt_arbiterindexes, node->mt_onconflict,
20562065
estate, node->canSetTag);
2066+
/* Revert ExecPrepareTupleRouting's state change. */
2067+
if (proute)
2068+
estate->es_result_relation_info = resultRelInfo;
20572069
break;
20582070
case CMD_UPDATE:
20592071
slot = ExecUpdate(node, tupleid, oldtuple, slot, planSlot,

‎src/include/nodes/execnodes.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -995,14 +995,18 @@ typedef struct ModifyTableState
995995
TupleTableSlot *mt_existing; /* slot to store existing target tuple in */
996996
List *mt_excludedtlist; /* the excluded pseudo relation's tlist */
997997
TupleTableSlot *mt_conflproj; /* CONFLICT ... SET ... projection target */
998-
struct PartitionTupleRouting *mt_partition_tuple_routing;
998+
999999
/* Tuple-routing support info */
1000-
struct TransitionCaptureState *mt_transition_capture;
1000+
struct PartitionTupleRouting *mt_partition_tuple_routing;
1001+
10011002
/* controls transition table population for specified operation */
1002-
struct TransitionCaptureState *mt_oc_transition_capture;
1003+
struct TransitionCaptureState *mt_transition_capture;
1004+
10031005
/* controls transition table population for INSERT...ON CONFLICT UPDATE */
1004-
TupleConversionMap **mt_per_subplan_tupconv_maps;
1006+
struct TransitionCaptureState *mt_oc_transition_capture;
1007+
10051008
/* Per plan map for tuple conversion from child to root */
1009+
TupleConversionMap **mt_per_subplan_tupconv_maps;
10061010
} ModifyTableState;
10071011

10081012
/* ----------------

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,32 @@ drop role regress_coldesc_role;
748748
drop table inserttest3;
749749
drop table brtrigpartcon;
750750
drop function brtrigpartcon1trigf();
751+
-- check that "do nothing" BR triggers work with tuple-routing (this checks
752+
-- that estate->es_result_relation_info is appropriately set/reset for each
753+
-- routed tuple)
754+
create table donothingbrtrig_test (a int, b text) partition by list (a);
755+
create table donothingbrtrig_test1 (b text, a int);
756+
create table donothingbrtrig_test2 (c text, b text, a int);
757+
alter table donothingbrtrig_test2 drop column c;
758+
create or replace function donothingbrtrig_func() returns trigger as $$begin raise notice 'b: %', new.b; return NULL; end$$ language plpgsql;
759+
create trigger donothingbrtrig1 before insert on donothingbrtrig_test1 for each row execute procedure donothingbrtrig_func();
760+
create trigger donothingbrtrig2 before insert on donothingbrtrig_test2 for each row execute procedure donothingbrtrig_func();
761+
alter table donothingbrtrig_test attach partition donothingbrtrig_test1 for values in (1);
762+
alter table donothingbrtrig_test attach partition donothingbrtrig_test2 for values in (2);
763+
insert into donothingbrtrig_test values (1, 'foo'), (2, 'bar');
764+
NOTICE: b: foo
765+
NOTICE: b: bar
766+
copy donothingbrtrig_test from stdout;
767+
NOTICE: b: baz
768+
NOTICE: b: qux
769+
select tableoid::regclass, * from donothingbrtrig_test;
770+
tableoid | a | b
771+
----------+---+---
772+
(0 rows)
773+
774+
-- cleanup
775+
drop table donothingbrtrig_test;
776+
drop function donothingbrtrig_func();
751777
-- check multi-column range partitioning with minvalue/maxvalue constraints
752778
create table mcrparted (a text, b int) partition by range(a, b);
753779
create table mcrparted1_lt_b partition of mcrparted for values from (minvalue, minvalue) to ('b', minvalue);

0 commit comments

Comments
 (0)
0