Hi,
Last message regarding this was by Robert on the original partitioning thread:
https://www.postgresql.org/message-id/CA%2BTgmoZjGzSM5WwnyapFaw3GxnDLWh7pm8Xiz8_QWQnUQy%3DSCA%40mail.gmail.com
Summary is: We decided in f1b4c771ea7 [1] 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 insert 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.
Attached rebased version of the patch that I had originally proposed
(summary above is the commit message). Robert thought it would be fine to
show the row formatted per partition rowtype, but would look better if we
could show the column names as well (remember that we're trying to account
for possible differences in the ordering of columns between the root table
and leaf partitions to which tuples are routed.)
Added this to PostgreSQL 10 open items list.
Thanks,
Amit
[1] https://git.postgresql.org/gitweb/?p=postgresql.git&a=commit&h=f1b4c77
>From fb3e65de8018b867f755fe145fe4759be5a0fb54 Mon Sep 17 00:00:00 2001
From: amit <amitlangot...@gmail.com>
Date: Fri, 31 Mar 2017 11:00:43 +0900
Subject: [PATCH] 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/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, 90 insertions(+), 25 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 0158eda591..b537f84278 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -2505,8 +2505,7 @@ CopyFrom(CopyState cstate)
for (;;)
{
- TupleTableSlot *slot,
- *oldslot;
+ TupleTableSlot *slot;
bool skip_tuple;
Oid loaded_oid = InvalidOid;
@@ -2548,7 +2547,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;
@@ -2650,7 +2648,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 f2995f2e7b..0f92bd49db 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1825,8 +1825,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);
@@ -1849,23 +1848,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);
@@ -1892,15 +1905,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);
@@ -1922,15 +1947,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
--
2.11.0
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
