On Thu, Apr 22, 2021 at 9:37 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangot...@gmail.com> writes:
> > FWIW, I think we should go ahead and apply the patches for the bug
> > reported here. Anyone who tries to project an updated tuple's system
> > columns using RETURNING are likely to face problems one way or
> > another, especially if they have partitioned tables containing
> > partitions of varying table AMs, but at least they won't face the bug
> > discussed here.
>
> Agreed, we should get this fixed in time for the next minor releases.
Thanks for taking a look at this.
> The issue no longer exists on HEAD, thanks to 86dc90056 having got
> rid of per-target-relation variance in the contents of planSlot.
> But we still need a fix for the back branches.
>
> So I looked over the v13 patch, and found a couple of things
> I didn't like:
>
> * I think what you did in ExecProcessReturning is buggy. It's
> not a great idea to have completely different processes for
> getting tableoid set in normal-relation vs foreign-relation
> cases, and in this case the foreign-relation case was simply
> wrong. Maybe the bug isn't reachable for lack of support of
> cross-partition motion with FDWs, but I'm not sure about that.
The two cases I see are some callers passing a valid 'tupleSlot' to
ExecProcessReturning() and some (just one!) not. The latter case
applies only to a foreign relations that are directly-modified and
relies on the FDWs having set econtext->ecxt_scantuple to the correct
slot. In the 1st case, the callers should already have made sure that
the correct tableoid is passed through 'tableSlot', although
Fujita-san had noticed in [1] that my earlier patch failed to do that
in the cross-partition update case. Along with fixing the problem of
my patch, he also proposed that we set the tableoid of the slot
assigned to ecxt_scantuple only in the 2nd case to save cycles.
I'm fine with leaving it the way it is as your updated patch does, but
I don't really see a bug being introduced. Actually, the code was
like what Fujita-san proposed we do before b8d71745eac changed it to
the current way:
@@ -166,20 +166,15 @@ ExecProcessReturning(ResultRelInfo *resultRelInfo,
/* Make tuple and any needed join variables available to ExecProject */
if (tupleSlot)
econtext->ecxt_scantuple = tupleSlot;
- else
- {
- HeapTuple tuple;
-
- /*
- * RETURNING expressions might reference the tableoid column, so
- * initialize t_tableOid before evaluating them.
- */
- Assert(!TupIsNull(econtext->ecxt_scantuple));
- tuple = ExecFetchSlotHeapTuple(econtext->ecxt_scantuple, true, NULL);
- tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
- }
econtext->ecxt_outertuple = planSlot;
+ /*
+ * RETURNING expressions might reference the tableoid column, so
+ * reinitialize tts_tableOid before evaluating them.
+ */
+ econtext->ecxt_scantuple->tts_tableOid =
+ RelationGetRelid(resultRelInfo->ri_RelationDesc);
+
/* Compute the RETURNING expressions */
return ExecProject(projectReturning);
}
> We really need to decouple the RETURNING expressions (which
> will belong to the source relation) from the value injected
> for tableoid (which will belong to the destination).
Yeah, that makes sense.
> * I really disliked the API change that ExecInsert is responsible
> for computing RETURNING except when it isn't. That's confusing
> and there's no good reason for it, since it's not really any
> easier to deal with the case at the call site than inside ExecInsert.
>
> In the attached revision I made ExecInsert handle RETURNING
> calculations by asking the callers to pass in the ResultRelInfo
> that should be used for the purpose.
That seems fine to me.
> We could alternatively
> have taken the responsibility for RETURNING out of ExecInsert
> altogether, making the callers call ExecProcessReturning.
> I think that might have netted out slightly simpler than this.
> But we're unlikely to apply such a change in HEAD, so it seemed
> better to keep the division of responsibilities the same as it
> is in other branches.
I agree.
> Thoughts?
The patch looks good to me, except one thing:
/*
+ * In a cross-partition UPDATE with RETURNING, we have to use the
+ * source partition's RETURNING list, because that matches the output
+ * of the planSlot, while the destination partition might have
+ * different resjunk columns. This means we have to map the
+ * destination slot back to the source's format so we can apply that
+ * RETURNING list. This is expensive, but it should be an uncommon
+ * corner case, so we won't spend much effort on making it fast.
+ */
+ if (returningRelInfo != resultRelInfo)
+ {
I think we should also add slot != srcSlot to this condition. The
uncommon corner case is the source and/or the destination partitions
having different column order than the root parent, thus requiring the
tuple to be converted during tuple routing using slots appropriate to
each relation, which causes 'slot' to end up different than 'srcSlot'.
But in the common case, where tuple descriptors match between all
tables involved, 'slot' should be the same as 'srcSlot'.
> (I've not looked at porting this to v12 or v11 yet.)
I did that; patches attached. (I haven't changed them to incorporate
the above comment though.)
--
Amit Langote
EDB: http://www.enterprisedb.com
[1]
https://www.postgresql.org/message-id/CAPmGK14QXD5Te_vwGgpuVWXRcrC%2Bd8FyWse0aHSqnDDSeeCRFQ%40mail.gmail.com
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index a0fd6c15d2..28b00df9e0 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -147,20 +147,26 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
* ExecProcessReturning --- evaluate a RETURNING list
*
* resultRelInfo: current result rel
+ * projectReturning: the projection to evaluate
+ * resultRelOid: result relation's OID
* tupleSlot: slot holding tuple actually inserted/updated/deleted
* planSlot: slot holding tuple returned by top subplan node
*
+ * In cross-partition UPDATE cases, projectReturning and planSlot are as
+ * for the source partition, and tupleSlot must conform to that. But
+ * resultRelOid is for the destination partition
+ *
* Note: If tupleSlot is NULL, the FDW should have already provided econtext's
* scan tuple.
*
* Returns a slot holding the result tuple
*/
static TupleTableSlot *
-ExecProcessReturning(ResultRelInfo *resultRelInfo,
+ExecProcessReturning(ProjectionInfo *projectReturning,
+ Oid resultRelOid,
TupleTableSlot *tupleSlot,
TupleTableSlot *planSlot)
{
- ProjectionInfo *projectReturning = resultRelInfo->ri_projectReturning;
ExprContext *econtext = projectReturning->pi_exprContext;
/*
@@ -177,12 +183,13 @@ ExecProcessReturning(ResultRelInfo *resultRelInfo,
HeapTuple tuple;
/*
- * RETURNING expressions might reference the tableoid column, so
- * initialize t_tableOid before evaluating them.
+ * RETURNING expressions might reference the tableoid column, so be
+ * sure we expose the desired OID, ie that of the real target
+ * relation.
*/
Assert(!TupIsNull(econtext->ecxt_scantuple));
tuple = ExecMaterializeSlot(econtext->ecxt_scantuple);
- tuple->t_tableOid = RelationGetRelid(resultRelInfo->ri_RelationDesc);
+ tuple->t_tableOid = resultRelOid;
}
econtext->ecxt_outertuple = planSlot;
@@ -256,6 +263,16 @@ ExecCheckTIDVisible(EState *estate,
* For INSERT, we have to insert the tuple into the target relation
* and insert appropriate tuples into the index relations.
*
+ * slot contains the new tuple value to be stored.
+ * planSlot is the output of the ModifyTable's subplan; we use it
+ * to access "junk" columns that are not going to be stored.
+ * In a cross-partition UPDATE, srcSlot is the slot that held the
+ * updated tuple for the source relation; otherwise it's NULL.
+ *
+ * returningRelInfo is the resultRelInfo for the source relation of a
+ * cross-partition UPDATE; otherwise it's the current result relation.
+ * We use it to process RETURNING lists, for reasons explained below.
+ *
* Returns RETURNING result if any, otherwise NULL.
* ----------------------------------------------------------------
*/
@@ -263,6 +280,8 @@ static TupleTableSlot *
ExecInsert(ModifyTableState *mtstate,
TupleTableSlot *slot,
TupleTableSlot *planSlot,
+ TupleTableSlot *srcSlot,
+ ResultRelInfo *returningRelInfo,
EState *estate,
bool canSetTag)
{
@@ -590,8 +609,61 @@ ExecInsert(ModifyTableState *mtstate,
ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate);
/* Process RETURNING if present */
- if (resultRelInfo->ri_projectReturning)
- result = ExecProcessReturning(resultRelInfo, slot, planSlot);
+ if (returningRelInfo->ri_projectReturning)
+ {
+ /*
+ * In a cross-partition UPDATE with RETURNING, we have to use the
+ * source partition's RETURNING list, because that matches the output
+ * of the planSlot, while the destination partition might have
+ * different resjunk columns. This means we have to map the
+ * destination slot back to the source's format so we can apply that
+ * RETURNING list. This is expensive, but it should be an uncommon
+ * corner case, so we won't spend much effort on making it fast.
+ */
+ if (returningRelInfo != resultRelInfo)
+ {
+ Relation destRelationDesc = resultRelInfo->ri_RelationDesc;
+ Relation srcRelationDesc = returningRelInfo->ri_RelationDesc;
+ TupleConversionMap *map;
+
+ map = convert_tuples_by_name(RelationGetDescr(resultRelationDesc),
+ RelationGetDescr(srcRelationDesc),
+ gettext_noop("could not convert row type"));
+ if (map)
+ {
+ /* We assume we can use srcSlot to hold the converted tuple */
+ HeapTuple origTuple = ExecMaterializeSlot(slot);
+ HeapTuple newTuple;
+
+ newTuple = do_convert_tuple(origTuple, map);
+
+ newTuple->t_self = newTuple->t_data->t_ctid =
+ origTuple->t_self;
+ newTuple->t_tableOid = origTuple->t_tableOid;
+
+ HeapTupleHeaderSetXmin(newTuple->t_data,
+ HeapTupleHeaderGetRawXmin(origTuple->t_data));
+ HeapTupleHeaderSetCmin(newTuple->t_data,
+ HeapTupleHeaderGetRawCommandId(origTuple->t_data));
+ HeapTupleHeaderSetXmax(newTuple->t_data,
+ InvalidTransactionId);
+
+ if (RelationGetDescr(destRelationDesc)->tdhasoid)
+ {
+ Assert(RelationGetDescr(srcRelationDesc)->tdhasoid);
+ HeapTupleSetOid(newTuple, HeapTupleGetOid(origTuple));
+ }
+
+ slot = ExecStoreTuple(newTuple, srcSlot, InvalidBuffer, true);
+
+ free_conversion_map(map);
+ }
+ }
+
+ result = ExecProcessReturning(returningRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelationDesc),
+ slot, planSlot);
+ }
return result;
}
@@ -891,7 +963,9 @@ ldelete:;
ExecStoreTuple(&deltuple, slot, InvalidBuffer, false);
}
- rslot = ExecProcessReturning(resultRelInfo, slot, planSlot);
+ rslot = ExecProcessReturning(resultRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelationDesc),
+ slot, planSlot);
/*
* Before releasing the target tuple again, make sure rslot has a
@@ -1068,6 +1142,7 @@ lreplace:;
{
bool tuple_deleted;
TupleTableSlot *ret_slot;
+ TupleTableSlot *orig_slot = slot;
TupleTableSlot *epqslot = NULL;
PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing;
int map_index;
@@ -1175,6 +1250,7 @@ lreplace:;
mtstate->rootResultRelInfo, slot);
ret_slot = ExecInsert(mtstate, slot, planSlot,
+ orig_slot, resultRelInfo,
estate, canSetTag);
/* Revert ExecPrepareTupleRouting's node change. */
@@ -1334,7 +1410,9 @@ lreplace:;
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
- return ExecProcessReturning(resultRelInfo, slot, planSlot);
+ return ExecProcessReturning(resultRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelationDesc),
+ slot, planSlot);
return NULL;
}
@@ -2067,7 +2145,9 @@ ExecModifyTable(PlanState *pstate)
* ExecProcessReturning by IterateDirectModify, so no need to
* provide it here.
*/
- slot = ExecProcessReturning(resultRelInfo, NULL, planSlot);
+ slot = ExecProcessReturning(resultRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelInfo->ri_RelationDesc),
+ NULL, planSlot);
estate->es_result_relation_info = saved_resultRelInfo;
return slot;
@@ -2157,6 +2237,7 @@ ExecModifyTable(PlanState *pstate)
slot = ExecPrepareTupleRouting(node, estate, proute,
resultRelInfo, slot);
slot = ExecInsert(node, slot, planSlot,
+ NULL, estate->es_result_relation_info,
estate, node->canSetTag);
/* Revert ExecPrepareTupleRouting's state change. */
if (proute)
diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out
index 2083345c8e..c35c0dd9f8 100644
--- a/src/test/regress/expected/update.out
+++ b/src/test/regress/expected/update.out
@@ -424,6 +424,46 @@ UPDATE range_parted set c = 95 WHERE a = 'b' and b > 10 and c > 100 returning (r
part_c_1_100 | b | 17 | 95 | 19 |
(6 rows)
+-- The following tests computing RETURNING when the source and the destination
+-- partitions of a UPDATE row movement operation have different tuple
+-- descriptors, which has been shown to be problematic in the cases where the
+-- RETURNING targetlist contains non-target relation attributes that are
+-- computed by referring to the source partition plan's output tuple. Also,
+-- a trigger on the destination relation may change the tuple, which must be
+-- reflected in the RETURNING output, so we test that too.
+CREATE TABLE part_c_1_c_20 (LIKE range_parted);
+ALTER TABLE part_c_1_c_20 DROP a, DROP b, ADD a text, ADD b bigint;
+ALTER TABLE range_parted ATTACH PARTITION part_c_1_c_20 FOR VALUES FROM ('c', 1) TO ('c', 20);
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN NEW.e := 'in trigfunc()'; RETURN NEW; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+ tableoid | a | b | c | d | e | x | y
+---------------+---+----+-----+---+---------------+---+----
+ part_c_1_c_20 | c | 1 | 1 | 1 | in trigfunc() | a | 1
+ part_c_1_c_20 | c | 10 | 200 | 1 | in trigfunc() | a | 10
+ part_c_1_c_20 | c | 12 | 96 | 1 | in trigfunc() | b | 12
+(3 rows)
+
+DROP TRIGGER trig ON part_c_1_c_20;
+DROP FUNCTION trigfunc;
+:init_range_parted;
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN RETURN NULL; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+ tableoid | a | b | c | d | e | x | y
+----------+---+---+---+---+---+---+---
+(0 rows)
+
+:show_data;
+ partname | a | b | c | d | e
+--------------+---+----+-----+----+---
+ part_c_1_100 | b | 13 | 97 | 2 |
+ part_d_15_20 | b | 15 | 105 | 16 |
+ part_d_15_20 | b | 17 | 105 | 19 |
+(3 rows)
+
+DROP TABLE part_c_1_c_20;
+DROP FUNCTION trigfunc;
-- Transition tables with update row movement
:init_range_parted;
CREATE FUNCTION trans_updatetrigfunc() RETURNS trigger LANGUAGE plpgsql AS
diff --git a/src/test/regress/sql/update.sql b/src/test/regress/sql/update.sql
index 8754ccb7b0..e6a61ace5b 100644
--- a/src/test/regress/sql/update.sql
+++ b/src/test/regress/sql/update.sql
@@ -223,6 +223,31 @@ DROP VIEW upview;
UPDATE range_parted set c = 95 WHERE a = 'b' and b > 10 and c > 100 returning (range_parted), *;
:show_data;
+-- The following tests computing RETURNING when the source and the destination
+-- partitions of a UPDATE row movement operation have different tuple
+-- descriptors, which has been shown to be problematic in the cases where the
+-- RETURNING targetlist contains non-target relation attributes that are
+-- computed by referring to the source partition plan's output tuple. Also,
+-- a trigger on the destination relation may change the tuple, which must be
+-- reflected in the RETURNING output, so we test that too.
+CREATE TABLE part_c_1_c_20 (LIKE range_parted);
+ALTER TABLE part_c_1_c_20 DROP a, DROP b, ADD a text, ADD b bigint;
+ALTER TABLE range_parted ATTACH PARTITION part_c_1_c_20 FOR VALUES FROM ('c', 1) TO ('c', 20);
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN NEW.e := 'in trigfunc()'; RETURN NEW; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+
+DROP TRIGGER trig ON part_c_1_c_20;
+DROP FUNCTION trigfunc;
+
+:init_range_parted;
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN RETURN NULL; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+:show_data;
+
+DROP TABLE part_c_1_c_20;
+DROP FUNCTION trigfunc;
-- Transition tables with update row movement
:init_range_parted;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index b65fc85a25..0cd3522152 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -149,34 +149,40 @@ ExecCheckPlanOutput(Relation resultRel, List *targetList)
/*
* ExecProcessReturning --- evaluate a RETURNING list
*
- * resultRelInfo: current result rel
+ * projectReturning: the projection to evaluate
+ * resultRelOid: result relation's OID
* tupleSlot: slot holding tuple actually inserted/updated/deleted
* planSlot: slot holding tuple returned by top subplan node
*
+ * In cross-partition UPDATE cases, projectReturning and planSlot are as
+ * for the source partition, and tupleSlot must conform to that. But
+ * resultRelOid is for the destination partition.
+ *
* Note: If tupleSlot is NULL, the FDW should have already provided econtext's
* scan tuple.
*
* Returns a slot holding the result tuple
*/
static TupleTableSlot *
-ExecProcessReturning(ResultRelInfo *resultRelInfo,
+ExecProcessReturning(ProjectionInfo *projectReturning,
+ Oid resultRelOid,
TupleTableSlot *tupleSlot,
TupleTableSlot *planSlot)
{
- ProjectionInfo *projectReturning = resultRelInfo->ri_projectReturning;
ExprContext *econtext = projectReturning->pi_exprContext;
/* Make tuple and any needed join variables available to ExecProject */
if (tupleSlot)
econtext->ecxt_scantuple = tupleSlot;
+ else
+ Assert(econtext->ecxt_scantuple);
econtext->ecxt_outertuple = planSlot;
/*
- * RETURNING expressions might reference the tableoid column, so
- * reinitialize tts_tableOid before evaluating them.
+ * RETURNING expressions might reference the tableoid column, so be sure
+ * we expose the desired OID, ie that of the real target relation.
*/
- econtext->ecxt_scantuple->tts_tableOid =
- RelationGetRelid(resultRelInfo->ri_RelationDesc);
+ econtext->ecxt_scantuple->tts_tableOid = resultRelOid;
/* Compute the RETURNING expressions */
return ExecProject(projectReturning);
@@ -343,6 +349,16 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot)
* For INSERT, we have to insert the tuple into the target relation
* and insert appropriate tuples into the index relations.
*
+ * slot contains the new tuple value to be stored.
+ * planSlot is the output of the ModifyTable's subplan; we use it
+ * to access "junk" columns that are not going to be stored.
+ * In a cross-partition UPDATE, srcSlot is the slot that held the
+ * updated tuple for the source relation; otherwise it's NULL.
+ *
+ * returningRelInfo is the resultRelInfo for the source relation of a
+ * cross-partition UPDATE; otherwise it's the current result relation.
+ * We use it to process RETURNING lists, for reasons explained below.
+ *
* Returns RETURNING result if any, otherwise NULL.
* ----------------------------------------------------------------
*/
@@ -350,6 +366,8 @@ static TupleTableSlot *
ExecInsert(ModifyTableState *mtstate,
TupleTableSlot *slot,
TupleTableSlot *planSlot,
+ TupleTableSlot *srcSlot,
+ ResultRelInfo *returningRelInfo,
EState *estate,
bool canSetTag)
{
@@ -652,8 +670,41 @@ ExecInsert(ModifyTableState *mtstate,
ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate);
/* Process RETURNING if present */
- if (resultRelInfo->ri_projectReturning)
- result = ExecProcessReturning(resultRelInfo, slot, planSlot);
+ if (returningRelInfo->ri_projectReturning)
+ {
+ /*
+ * In a cross-partition UPDATE with RETURNING, we have to use the
+ * source partition's RETURNING list, because that matches the output
+ * of the planSlot, while the destination partition might have
+ * different resjunk columns. This means we have to map the
+ * destination slot back to the source's format so we can apply that
+ * RETURNING list. This is expensive, but it should be an uncommon
+ * corner case, so we won't spend much effort on making it fast.
+ */
+ if (returningRelInfo != resultRelInfo)
+ {
+ Relation srcRelationDesc = returningRelInfo->ri_RelationDesc;
+ AttrNumber *map;
+
+ map = convert_tuples_by_name_map_if_req(RelationGetDescr(resultRelationDesc),
+ RelationGetDescr(srcRelationDesc),
+ gettext_noop("could not convert row type"));
+ if (map)
+ {
+ /* We assume we can use srcSlot to hold the converted tuple */
+ TupleTableSlot *origSlot = slot;
+
+ slot = execute_attr_map_slot(map, slot, srcSlot);
+ slot->tts_tid = origSlot->tts_tid;
+ slot->tts_tableOid = origSlot->tts_tableOid;
+ pfree(map);
+ }
+ }
+
+ result = ExecProcessReturning(returningRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelationDesc),
+ slot, planSlot);
+ }
return result;
}
@@ -1002,7 +1053,9 @@ ldelete:;
}
}
- rslot = ExecProcessReturning(resultRelInfo, slot, planSlot);
+ rslot = ExecProcessReturning(resultRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelationDesc),
+ slot, planSlot);
/*
* Before releasing the target tuple again, make sure rslot has a
@@ -1178,6 +1231,7 @@ lreplace:;
{
bool tuple_deleted;
TupleTableSlot *ret_slot;
+ TupleTableSlot *orig_slot = slot;
TupleTableSlot *epqslot = NULL;
PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing;
int map_index;
@@ -1284,6 +1338,7 @@ lreplace:;
mtstate->rootResultRelInfo, slot);
ret_slot = ExecInsert(mtstate, slot, planSlot,
+ orig_slot, resultRelInfo,
estate, canSetTag);
/* Revert ExecPrepareTupleRouting's node change. */
@@ -1480,7 +1535,9 @@ lreplace:;
/* Process RETURNING if present */
if (resultRelInfo->ri_projectReturning)
- return ExecProcessReturning(resultRelInfo, slot, planSlot);
+ return ExecProcessReturning(resultRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelationDesc),
+ slot, planSlot);
return NULL;
}
@@ -2130,7 +2187,9 @@ ExecModifyTable(PlanState *pstate)
* ExecProcessReturning by IterateDirectModify, so no need to
* provide it here.
*/
- slot = ExecProcessReturning(resultRelInfo, NULL, planSlot);
+ slot = ExecProcessReturning(resultRelInfo->ri_projectReturning,
+ RelationGetRelid(resultRelInfo->ri_RelationDesc),
+ NULL, planSlot);
estate->es_result_relation_info = saved_resultRelInfo;
return slot;
@@ -2220,6 +2279,7 @@ ExecModifyTable(PlanState *pstate)
slot = ExecPrepareTupleRouting(node, estate, proute,
resultRelInfo, slot);
slot = ExecInsert(node, slot, planSlot,
+ NULL, estate->es_result_relation_info,
estate, node->canSetTag);
/* Revert ExecPrepareTupleRouting's state change. */
if (proute)
diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out
index a24ecd61df..4a0eb9a315 100644
--- a/src/test/regress/expected/update.out
+++ b/src/test/regress/expected/update.out
@@ -447,6 +447,46 @@ UPDATE range_parted set c = 95 WHERE a = 'b' and b > 10 and c > 100 returning (r
part_c_1_100 | b | 17 | 95 | 19 |
(6 rows)
+-- The following tests computing RETURNING when the source and the destination
+-- partitions of a UPDATE row movement operation have different tuple
+-- descriptors, which has been shown to be problematic in the cases where the
+-- RETURNING targetlist contains non-target relation attributes that are
+-- computed by referring to the source partition plan's output tuple. Also,
+-- a trigger on the destination relation may change the tuple, which must be
+-- reflected in the RETURNING output, so we test that too.
+CREATE TABLE part_c_1_c_20 (LIKE range_parted);
+ALTER TABLE part_c_1_c_20 DROP a, DROP b, ADD a text, ADD b bigint;
+ALTER TABLE range_parted ATTACH PARTITION part_c_1_c_20 FOR VALUES FROM ('c', 1) TO ('c', 20);
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN NEW.e := 'in trigfunc()'; RETURN NEW; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+ tableoid | a | b | c | d | e | x | y
+---------------+---+----+-----+---+---------------+---+----
+ part_c_1_c_20 | c | 1 | 1 | 1 | in trigfunc() | a | 1
+ part_c_1_c_20 | c | 10 | 200 | 1 | in trigfunc() | a | 10
+ part_c_1_c_20 | c | 12 | 96 | 1 | in trigfunc() | b | 12
+(3 rows)
+
+DROP TRIGGER trig ON part_c_1_c_20;
+DROP FUNCTION trigfunc;
+:init_range_parted;
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN RETURN NULL; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+ tableoid | a | b | c | d | e | x | y
+----------+---+---+---+---+---+---+---
+(0 rows)
+
+:show_data;
+ partname | a | b | c | d | e
+--------------+---+----+-----+----+---
+ part_c_1_100 | b | 13 | 97 | 2 |
+ part_d_15_20 | b | 15 | 105 | 16 |
+ part_d_15_20 | b | 17 | 105 | 19 |
+(3 rows)
+
+DROP TABLE part_c_1_c_20;
+DROP FUNCTION trigfunc;
-- Transition tables with update row movement
:init_range_parted;
CREATE FUNCTION trans_updatetrigfunc() RETURNS trigger LANGUAGE plpgsql AS
diff --git a/src/test/regress/sql/update.sql b/src/test/regress/sql/update.sql
index bb9c24e40f..5ec58eece2 100644
--- a/src/test/regress/sql/update.sql
+++ b/src/test/regress/sql/update.sql
@@ -238,6 +238,31 @@ DROP VIEW upview;
UPDATE range_parted set c = 95 WHERE a = 'b' and b > 10 and c > 100 returning (range_parted), *;
:show_data;
+-- The following tests computing RETURNING when the source and the destination
+-- partitions of a UPDATE row movement operation have different tuple
+-- descriptors, which has been shown to be problematic in the cases where the
+-- RETURNING targetlist contains non-target relation attributes that are
+-- computed by referring to the source partition plan's output tuple. Also,
+-- a trigger on the destination relation may change the tuple, which must be
+-- reflected in the RETURNING output, so we test that too.
+CREATE TABLE part_c_1_c_20 (LIKE range_parted);
+ALTER TABLE part_c_1_c_20 DROP a, DROP b, ADD a text, ADD b bigint;
+ALTER TABLE range_parted ATTACH PARTITION part_c_1_c_20 FOR VALUES FROM ('c', 1) TO ('c', 20);
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN NEW.e := 'in trigfunc()'; RETURN NEW; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+
+DROP TRIGGER trig ON part_c_1_c_20;
+DROP FUNCTION trigfunc;
+
+:init_range_parted;
+CREATE FUNCTION trigfunc () RETURNS TRIGGER LANGUAGE plpgsql as $$ BEGIN RETURN NULL; END; $$;
+CREATE TRIGGER trig BEFORE INSERT ON part_c_1_c_20 FOR EACH ROW EXECUTE FUNCTION trigfunc();
+UPDATE range_parted r set a = 'c' FROM (VALUES ('a', 1), ('a', 10), ('b', 12)) s(x, y) WHERE s.x = r.a AND s.y = r.b RETURNING tableoid::regclass, *;
+:show_data;
+
+DROP TABLE part_c_1_c_20;
+DROP FUNCTION trigfunc;
-- Transition tables with update row movement
:init_range_parted;
