From dfa3a23f426ea71ca8b73c114a732134d9acbefb Mon Sep 17 00:00:00 2001 From: amitlan Date: Thu, 11 Jun 2020 09:54:08 +0900 Subject: [PATCH v1] Fix a bug with RETURNING when UPDATE "moves" tuple Currently, the RETURNING list is projected by ExecInsert() that is running on the destination partition and hence using that partition's version of the RETURNING projection. If the RETURNING list contains some non-target relation's columns, they refer to the corresponding columns in the plan's output tuple ('planSlot'), whose descriptor is based on the *source* result relation's descriptor, which can lead to attribute mismatch errors when projecting with the destination relation's RETURNING projection. Fix is to perform the projection in ExecUpdate using the source relation's RETURNING projection after returning from ExecInsert(). --- src/backend/executor/nodeModifyTable.c | 51 ++++++++++++++++++++++++++++++---- src/test/regress/expected/update.out | 18 ++++++++++++ src/test/regress/sql/update.sql | 11 ++++++++ 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 20a4c47..88c7bea 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -368,7 +368,7 @@ ExecComputeStoredGenerated(EState *estate, TupleTableSlot *slot, CmdType cmdtype * For INSERT, we have to insert the tuple into the target relation * and insert appropriate tuples into the index relations. * - * Returns RETURNING result if any, otherwise NULL. + * Returns RETURNING result if any and requested, otherwise NULL. * ---------------------------------------------------------------- */ static TupleTableSlot * @@ -376,7 +376,8 @@ ExecInsert(ModifyTableState *mtstate, TupleTableSlot *slot, TupleTableSlot *planSlot, EState *estate, - bool canSetTag) + bool canSetTag, + bool processReturning) { ResultRelInfo *resultRelInfo; Relation resultRelationDesc; @@ -677,7 +678,7 @@ ExecInsert(ModifyTableState *mtstate, ExecWithCheckOptions(WCO_VIEW_CHECK, resultRelInfo, slot, estate); /* Process RETURNING if present */ - if (resultRelInfo->ri_projectReturning) + if (processReturning && resultRelInfo->ri_projectReturning) result = ExecProcessReturning(resultRelInfo, slot, planSlot); return result; @@ -1202,11 +1203,13 @@ lreplace:; if (partition_constraint_failed) { bool tuple_deleted; + TupleTableSlot *orig_slot = slot; TupleTableSlot *ret_slot; TupleTableSlot *epqslot = NULL; PartitionTupleRouting *proute = mtstate->mt_partition_tuple_routing; int map_index; TupleConversionMap *tupconv_map; + ResultRelInfo *destRel; /* * Disallow an INSERT ON CONFLICT DO UPDATE that causes the @@ -1308,8 +1311,18 @@ lreplace:; slot = ExecPrepareTupleRouting(mtstate, estate, proute, mtstate->rootResultRelInfo, slot); - ret_slot = ExecInsert(mtstate, slot, planSlot, - estate, canSetTag); + /* + * It can be wrong for ExecInsert() to process RETURNING, because + * it will use the "destination" partition's projection, which may + * not work. That's because any column in the RETURNING list that + * references the tuple in planSlot, which is based on the + * "source" partition's reltype, may cause an attribute mismtach + * error, if the "source" and "destination" reltypes don't match. + */ + ret_slot = ExecInsert(mtstate, slot, planSlot, estate, canSetTag, + false); + /* The "destination" partition. */ + destRel = estate->es_result_relation_info; /* Revert ExecPrepareTupleRouting's node change. */ estate->es_result_relation_info = resultRelInfo; @@ -1319,6 +1332,31 @@ lreplace:; mtstate->mt_transition_capture->tcs_map = saved_tcs_map; } + /* Process RETURNING using the "source" relation's projection. */ + if (resultRelInfo->ri_projectReturning) + { + AttrMap *map; + + /* + * We can't use the original tuple, because INSERT triggers + * may have changed it. However, we may need to convert the + * tuple in 'slot' to the source result relation's reltype as + * it has been made by the code above to contain a tuple in the + * destination partition reltype. + * + * XXX - it's not great that we are having to rebuild the map + * from scratch for every tuple in the case it's needed! + */ + if (slot != orig_slot) + { + map = build_attrmap_by_name_if_req(RelationGetDescr(destRel->ri_RelationDesc), + RelationGetDescr(resultRelInfo->ri_RelationDesc)); + if (map) + slot = execute_attr_map_slot(map, slot, orig_slot); + } + return ExecProcessReturning(resultRelInfo, slot, planSlot); + } + return ret_slot; } @@ -2244,7 +2282,8 @@ ExecModifyTable(PlanState *pstate) slot = ExecPrepareTupleRouting(node, estate, proute, resultRelInfo, slot); slot = ExecInsert(node, slot, planSlot, - estate, node->canSetTag); + estate, node->canSetTag, + true); /* Revert ExecPrepareTupleRouting's state change. */ if (proute) estate->es_result_relation_info = resultRelInfo; diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out index bf939d7..dcf92dc 100644 --- a/src/test/regress/expected/update.out +++ b/src/test/regress/expected/update.out @@ -445,6 +445,24 @@ 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) +-- RETURNING with non-target relation present has been shown to be problematic +-- when source and destination result relations have different number of +-- attributes (considering dropped attributes that is) +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 nooptrig 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_a_1_a_10 | c | 1 | 1 | 1 | in trigfunc() | a | 1 + part_a_10_a_20 | c | 10 | 200 | 1 | in trigfunc() | a | 10 + part_c_1_100 | c | 12 | 96 | 1 | in trigfunc() | b | 12 +(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 8c558a7..93e1305 100644 --- a/src/test/regress/sql/update.sql +++ b/src/test/regress/sql/update.sql @@ -236,6 +236,17 @@ DROP VIEW upview; UPDATE range_parted set c = 95 WHERE a = 'b' and b > 10 and c > 100 returning (range_parted), *; :show_data; +-- RETURNING with non-target relation present has been shown to be problematic +-- when source and destination result relations have different number of +-- attributes (considering dropped attributes that is) +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 nooptrig 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 TABLE part_c_1_c_20; +DROP FUNCTION trigfunc; -- Transition tables with update row movement :init_range_parted; -- 1.8.3.1