From c0a8ae7be392aa09dd7e148ff662013e8e148893 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 10 Apr 2017 12:20:08 -0400 Subject: [PATCH] Fix reporting of violations in ExecConstraints, again. We decided in f1b4c771ea74f42447dccaed42ffcdcccf3aa694 to pass the original slot to ExecConstraints(), but that breaks when there are BEFORE ROW triggers involved. So we need to do reverse-map the tuples back to the original descriptor instead, as Amit originally proposed. Amit Langote, reviewed by Ashutosh Bapat. One overlooked comment fixed by me. Discussion: http://postgr.es/m/b3a17254-6849-e542-2353-bde4e880b6a4@lab.ntt.co.jp --- src/backend/commands/copy.c | 6 +-- src/backend/executor/execMain.c | 58 +++++++++++++++++++++----- src/backend/executor/execReplication.c | 4 +- src/backend/executor/nodeModifyTable.c | 7 ++-- src/include/executor/executor.h | 3 +- src/test/regress/expected/insert.out | 21 ++++++++-- src/test/regress/sql/insert.sql | 21 +++++++++- 7 files changed, 92 insertions(+), 28 deletions(-) diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c index 8c58808686..73677be59e 100644 --- a/src/backend/commands/copy.c +++ b/src/backend/commands/copy.c @@ -2506,8 +2506,7 @@ CopyFrom(CopyState cstate) for (;;) { - TupleTableSlot *slot, - *oldslot; + TupleTableSlot *slot; bool skip_tuple; Oid loaded_oid = InvalidOid; @@ -2549,7 +2548,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; @@ -2651,7 +2649,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 920b12072f..5c12fb457d 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -1824,14 +1824,12 @@ ExecPartitionCheck(ResultRelInfo *resultRelInfo, TupleTableSlot *slot, * the partition constraint, if any. * * Note: 'slot' contains the tuple to check the constraints of, which may - * have been converted from the original input tuple after tuple routing, - * while 'orig_slot' contains the original tuple to be shown in the message, - * if an error occurs. + * have been converted from the original input tuple after tuple routing. + * 'resultRelInfo' is the original result relation, before tuple routing. */ void ExecConstraints(ResultRelInfo *resultRelInfo, - TupleTableSlot *slot, TupleTableSlot *orig_slot, - EState *estate) + TupleTableSlot *slot, EState *estate) { Relation rel = resultRelInfo->ri_RelationDesc; TupleDesc tupdesc = RelationGetDescr(rel); @@ -1854,23 +1852,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 + * If the tuple has been 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 + * rowtype 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); @@ -1897,15 +1909,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); @@ -1927,15 +1951,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/execReplication.c b/src/backend/executor/execReplication.c index f20d728ad5..327a0bad38 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -389,7 +389,7 @@ ExecSimpleRelationInsert(EState *estate, TupleTableSlot *slot) /* Check the constraints of the tuple */ if (rel->rd_att->constr) - ExecConstraints(resultRelInfo, slot, slot, estate); + ExecConstraints(resultRelInfo, slot, estate); /* Store the slot into tuple that we can inspect. */ tuple = ExecMaterializeSlot(slot); @@ -448,7 +448,7 @@ ExecSimpleRelationUpdate(EState *estate, EPQState *epqstate, /* Check the constraints of the tuple */ if (rel->rd_att->constr) - ExecConstraints(resultRelInfo, slot, slot, estate); + ExecConstraints(resultRelInfo, slot, estate); /* Store the slot into tuple that we can write. */ tuple = ExecMaterializeSlot(slot); diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 0b524e0b7c..00b736c22c 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -263,8 +263,7 @@ ExecInsert(ModifyTableState *mtstate, Relation resultRelationDesc; Oid newId; List *recheckIndexes = NIL; - TupleTableSlot *oldslot = slot, - *result = NULL; + TupleTableSlot *result = NULL; /* * get the heap tuple out of the tuple table slot, making sure we have a @@ -435,7 +434,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) { @@ -993,7 +992,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 d3849b93eb..f7f3189a1a 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -186,8 +186,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 7fafa98212..6f34b1c640 100644 --- a/src/test/regress/expected/insert.out +++ b/src/test/regress/expected/insert.out @@ -354,11 +354,26 @@ ERROR: no partition of relation "mlparted1" found for row DETAIL: Partition key of the failing row contains ((b + 0)) = (5). truncate mlparted; alter table mlparted add constraint check_b check (b = 3); --- check that correct input row is shown when constraint check_b fails on mlparted11 --- after "(1, 2)" is routed to it +-- have a BR trigger modify the row such that the check_b is violated +create function mlparted11_trig_fn() +returns trigger AS +$$ +begin + NEW.b := 4; + return NEW; +end; +$$ +language plpgsql; +create trigger mlparted11_trig before insert ON mlparted11 + for each row execute procedure mlparted11_trig_fn(); +-- check that the correct row is shown when constraint check_b fails after +-- "(1, 2)" is routed to mlparted11 (actually "(1, 4)" would be shown due +-- to the BR trigger mlparted11_trig_fn) insert into mlparted values (1, 2); ERROR: new row for relation "mlparted11" violates check constraint "check_b" -DETAIL: Failing row contains (1, 2). +DETAIL: Failing row contains (1, 4). +drop trigger mlparted11_trig on mlparted11; +drop function mlparted11_trig_fn(); -- check that inserting into an internal partition successfully results in -- checking its partition constraint before inserting into the leaf partition -- selected by tuple-routing diff --git a/src/test/regress/sql/insert.sql b/src/test/regress/sql/insert.sql index f9c00705a2..020854c960 100644 --- a/src/test/regress/sql/insert.sql +++ b/src/test/regress/sql/insert.sql @@ -217,9 +217,26 @@ insert into mlparted (a, b) values (1, 5); truncate mlparted; alter table mlparted add constraint check_b check (b = 3); --- check that correct input row is shown when constraint check_b fails on mlparted11 --- after "(1, 2)" is routed to it + +-- have a BR trigger modify the row such that the check_b is violated +create function mlparted11_trig_fn() +returns trigger AS +$$ +begin + NEW.b := 4; + return NEW; +end; +$$ +language plpgsql; +create trigger mlparted11_trig before insert ON mlparted11 + for each row execute procedure mlparted11_trig_fn(); + +-- check that the correct row is shown when constraint check_b fails after +-- "(1, 2)" is routed to mlparted11 (actually "(1, 4)" would be shown due +-- to the BR trigger mlparted11_trig_fn) insert into mlparted values (1, 2); +drop trigger mlparted11_trig on mlparted11; +drop function mlparted11_trig_fn(); -- check that inserting into an internal partition successfully results in -- checking its partition constraint before inserting into the leaf partition