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

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 8c5880868635fca3f5a07b231253ffb31a802ada..73677be59e03dc2d79806b07125e176a7671ec4e 100644 (file)
--- 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 920b12072fb56eedf203f6c30fee865c78a2d9c3..5c12fb457d586014bad54d07b3392a2f5fdf870a 100644 (file)
--- 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 f20d728ad5a4d6ec398bc77850fe07a41c40f867..327a0bad3886f6d2eba88c874f2d6fed13c80395 100644 (file)
--- 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 0b524e0b7cfb1634707f5ace7477dade752db42d..00b736c22c444b1f6cd9975c328366ec748b75bc 100644 (file)
--- 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 d3849b93eb19c49226c48b3f0fbb0323db1846a6..f7f3189a1a096d2ccefafe3ba9a2015c97d22c0d 100644 (file)
--- 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 7fafa982126dda29a1c51aa7144200c0462182fa..6f34b1c640684274be4743c0b424df4fdacd2fe1 100644 (file)
--- 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 f9c00705a2204086cd4f83e5b09e1f50b9d46f48..020854c960ee5056afee388b4817aa322babe8ba 100644 (file)
--- 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