Re: [HACKERS] tuple-routing and constraint violation error message, revisited
On Mon, Apr 10, 2017 at 8:14 AM, Ashutosh Bapatwrote: > On Fri, Mar 31, 2017 at 7:43 AM, Amit Langote > wrote: >> 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. > > The changes look good to me. Now, ExecConstraint() has three blocks > which are almost similar, differing only in the constraints checked > and the error message. It was manageable without partitioning and may > be it's still manageable, but it's certainly being pushed to the > limits. May be we should refactor error reporting code into a separate > function and call it in those three places. Yeah, possibly. Thanks for the review. I have committed the patch. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] tuple-routing and constraint violation error message, revisited
On Fri, Mar 31, 2017 at 7:43 AM, Amit Langotewrote: > 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. The changes look good to me. Now, ExecConstraint() has three blocks which are almost similar, differing only in the constraints checked and the error message. It was manageable without partitioning and may be it's still manageable, but it's certainly being pushed to the limits. May be we should refactor error reporting code into a separate function and call it in those three places. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] tuple-routing and constraint violation error message, revisited
On Fri, Mar 31, 2017 at 11:13:03AM +0900, Amit Langote wrote: > 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. [Action required within three days. This is a generic notification.] The above-described topic is currently a PostgreSQL 10 open item. Robert, since you committed the patch believed to have created it, you own this open item. If some other commit is more relevant or if this does not belong as a v10 open item, please let us know. Otherwise, please observe the policy on open item ownership[1] and send a status update within three calendar days of this message. Include a date for your subsequent status update. Testers may discover new open items at any time, and I want to plan to get them all fixed well in advance of shipping v10. Consequently, I will appreciate your efforts toward speedy resolution. Thanks. [1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] tuple-routing and constraint violation error message, revisited
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=commit=f1b4c77 >From fb3e65de8018b867f755fe145fe4759be5a0fb54 Mon Sep 17 00:00:00 2001 From: amitDate: 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"));