From e408234633c01817d6a2313fdbdccdb4f0057c1e Mon Sep 17 00:00:00 2001 From: amit Date: Fri, 6 Jan 2017 15:53:10 +0900 Subject: [PATCH 1/7] Fix reporting of violation in ExecConstraints, again We decided in f1b4c771ea74f42447dccaed42ffcdcccf3aa694 that passing the original slot (one containing the tuple formatted per root partitioned table's tupdesc) to ExecConstraints(), but that breaks certain cases. Imagine what would happen if a BR trigger changed the tuple - the original slot would not contain those changes. So, it seems better to convert (if necessary) the tuple formatted per partition tupdesc after tuple-routing back to the root table's format and use the converted tuple to make val_desc shown in the message if an error occurs. --- src/backend/commands/copy.c | 6 ++-- src/backend/executor/execMain.c | 53 +++++++++++++++++++++++++++++----- src/backend/executor/nodeModifyTable.c | 5 ++-- src/include/executor/executor.h | 3 +- src/test/regress/expected/insert.out | 18 ++++++++++-- src/test/regress/sql/insert.sql | 17 ++++++++++- 6 files changed, 82 insertions(+), 20 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index f56b2ac49b..65eb167087 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -2491,8 +2491,7 @@ CopyFrom(CopyState cstate) for (;;) { - TupleTableSlot *slot, - *oldslot; + TupleTableSlot *slot; bool skip_tuple; Oid loaded_oid = InvalidOid; @@ -2534,7 +2533,6 @@ CopyFrom(CopyState cstate) ExecStoreTuple(tuple, slot, InvalidBuffer, false); /* Determine the partition to heap_insert the tuple into */ - oldslot = slot; if (cstate->partition_dispatch_info) { int leaf_part_index; @@ -2625,7 +2623,7 @@ CopyFrom(CopyState cstate) /* Check the constraints of the tuple */ if (cstate->rel->rd_att->constr || resultRelInfo->ri_PartitionCheck) - ExecConstraints(resultRelInfo, slot, oldslot, estate); + ExecConstraints(resultRelInfo, slot, estate); if (useHeapMultiInsert) { diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index ff277d300a..332c54c819 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1763,8 +1763,7 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, */ void ExecConstraints(ResultRelInfo *resultRelInfo, - TupleTableSlot *slot, TupleTableSlot *orig_slot, - EState *estate) + TupleTableSlot *slot, EState *estate) { Relation rel = resultRelInfo->ri_RelationDesc; TupleDesc tupdesc = RelationGetDescr(rel); @@ -1787,23 +1786,37 @@ ExecConstraints(ResultRelInfo *resultRelInfo, { char *val_desc; Relation orig_rel = rel; - TupleDesc orig_tupdesc = tupdesc; + TupleDesc orig_tupdesc = RelationGetDescr(rel); /* - * choose the correct relation to build val_desc from the - * tuple contained in orig_slot + * In case where the tuple is routed, it's been converted + * to the partition's rowtype, which might differ from the + * root table's. We must convert it back to the root table's + * type so that val_desc shown error message matches the + * input tuple. */ if (resultRelInfo->ri_PartitionRoot) { + HeapTuple tuple = ExecFetchSlotTuple(slot); + TupleConversionMap *map; + rel = resultRelInfo->ri_PartitionRoot; tupdesc = RelationGetDescr(rel); + /* a reverse map */ + map = convert_tuples_by_name(orig_tupdesc, tupdesc, + gettext_noop("could not convert row type")); + if (map != NULL) + { + tuple = do_convert_tuple(tuple, map); + ExecStoreTuple(tuple, slot, InvalidBuffer, false); + } } insertedCols = GetInsertedColumns(resultRelInfo, estate); updatedCols = GetUpdatedColumns(resultRelInfo, estate); modifiedCols = bms_union(insertedCols, updatedCols); val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), - orig_slot, + slot, tupdesc, modifiedCols, 64); @@ -1830,15 +1843,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo, /* See the comment above. */ if (resultRelInfo->ri_PartitionRoot) { + HeapTuple tuple = ExecFetchSlotTuple(slot); + TupleDesc old_tupdesc = RelationGetDescr(rel); + TupleConversionMap *map; + rel = resultRelInfo->ri_PartitionRoot; tupdesc = RelationGetDescr(rel); + /* a reverse map */ + map = convert_tuples_by_name(old_tupdesc, tupdesc, + gettext_noop("could not convert row type")); + if (map != NULL) + { + tuple = do_convert_tuple(tuple, map); + ExecStoreTuple(tuple, slot, InvalidBuffer, false); + } } insertedCols = GetInsertedColumns(resultRelInfo, estate); updatedCols = GetUpdatedColumns(resultRelInfo, estate); modifiedCols = bms_union(insertedCols, updatedCols); val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), - orig_slot, + slot, tupdesc, modifiedCols, 64); @@ -1860,15 +1885,27 @@ ExecConstraints(ResultRelInfo *resultRelInfo, /* See the comment above. */ if (resultRelInfo->ri_PartitionRoot) { + HeapTuple tuple = ExecFetchSlotTuple(slot); + TupleDesc old_tupdesc = RelationGetDescr(rel); + TupleConversionMap *map; + rel = resultRelInfo->ri_PartitionRoot; tupdesc = RelationGetDescr(rel); + /* a reverse map */ + map = convert_tuples_by_name(old_tupdesc, tupdesc, + gettext_noop("could not convert row type")); + if (map != NULL) + { + tuple = do_convert_tuple(tuple, map); + ExecStoreTuple(tuple, slot, InvalidBuffer, false); + } } insertedCols = GetInsertedColumns(resultRelInfo, estate); updatedCols = GetUpdatedColumns(resultRelInfo, estate); modifiedCols = bms_union(insertedCols, updatedCols); val_desc = ExecBuildSlotValueDescription(RelationGetRelid(rel), - orig_slot, + slot, tupdesc, modifiedCols, 64); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 4692427e60..23e04893b8 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -262,7 +262,6 @@ ExecInsert(ModifyTableState *mtstate, Relation resultRelationDesc; Oid newId; List *recheckIndexes = NIL; - TupleTableSlot *oldslot = slot; /* * get the heap tuple out of the tuple table slot, making sure we have a @@ -433,7 +432,7 @@ ExecInsert(ModifyTableState *mtstate, * Check the constraints of the tuple */ if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck) - ExecConstraints(resultRelInfo, slot, oldslot, estate); + ExecConstraints(resultRelInfo, slot, estate); if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0) { @@ -994,7 +993,7 @@ lreplace:; * tuple-routing is performed here, hence the slot remains unchanged. */ if (resultRelationDesc->rd_att->constr || resultRelInfo->ri_PartitionCheck) - ExecConstraints(resultRelInfo, slot, slot, estate); + ExecConstraints(resultRelInfo, slot, estate); /* * replace the heap tuple diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index b9c7f72903..3e8d64686e 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -195,8 +195,7 @@ extern void InitResultRelInfo(ResultRelInfo *resultRelInfo, extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid); extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids); extern void ExecConstraints(ResultRelInfo *resultRelInfo, - TupleTableSlot *slot, TupleTableSlot *orig_slot, - EState *estate); + TupleTableSlot *slot, EState *estate); extern void ExecWithCheckOptions(WCOKind kind, ResultRelInfo *resultRelInfo, TupleTableSlot *slot, EState *estate); extern LockTupleMode ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo); diff --git a/src/test/regress/expected/insert.out b/src/test/regress/expected/insert.out index ca3134c34c..501d50eeac 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -335,10 +335,24 @@ select tableoid::regclass, * from p; truncate p; alter table p add constraint check_b check (b = 3); +create function p11_trig_fn() +returns trigger AS +$$ +begin + NEW.b := 4; + return NEW; +end; +$$ +language plpgsql; +create trigger p11_trig before insert ON p11 + for each row execute procedure p11_trig_fn(); -- check that correct input row is shown when constraint check_b fails on p11 --- after "(1, 2)" is routed to it +-- after "(1, 2)" is routed to it (actually "(1, 4)" would be shown due to the +-- BR trigger p11_trig_fn) insert into p values (1, 2); ERROR: new row for relation "p11" violates check constraint "check_b" -DETAIL: Failing row contains (1, 2). +DETAIL: Failing row contains (1, 4). +drop trigger p11_trig on p11; +drop function p11_trig_fn(); -- cleanup drop table p, p1, p11; diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index 09c9879da1..22aa94e181 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -197,9 +197,24 @@ select tableoid::regclass, * from p; truncate p; alter table p add constraint check_b check (b = 3); +create function p11_trig_fn() +returns trigger AS +$$ +begin + NEW.b := 4; + return NEW; +end; +$$ +language plpgsql; +create trigger p11_trig before insert ON p11 + for each row execute procedure p11_trig_fn(); + -- check that correct input row is shown when constraint check_b fails on p11 --- after "(1, 2)" is routed to it +-- after "(1, 2)" is routed to it (actually "(1, 4)" would be shown due to the +-- BR trigger p11_trig_fn) insert into p values (1, 2); +drop trigger p11_trig on p11; +drop function p11_trig_fn(); -- cleanup drop table p, p1, p11; -- 2.11.0