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:

/messages/by-id/CA+TgmoZjGzSM5WwnyapFaw3GxnDLWh7pm8Xiz8_QWQnUQy=SCA@mail.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]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com 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]: /messages/by-id/20170404140717.GA2675809@tornado.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

On Fri, Mar 31, 2017 at 7:43 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:

Hi,

Last message regarding this was by Robert on the original partitioning thread:

/messages/by-id/CA+TgmoZjGzSM5WwnyapFaw3GxnDLWh7pm8Xiz8_QWQnUQy=SCA@mail.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

On Mon, Apr 10, 2017 at 8:14 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

On Fri, Mar 31, 2017 at 7:43 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> 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