On Thu, 23 Jan 2020 at 10:20, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Jan 22, 2020 at 8:48 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > > > Alvaro Herrera <alvhe...@2ndquadrant.com> writes: > > > I wonder if we shouldn't be using errtablecol() here instead of (in > > > addition to?) patching the errmsg() to include the table name. > > > > > Discussion: If we really like having the table names in errtable(), then > > > we should have psql display it by default, and other tools will follow > > > suit; in that case we should remove the table name from error messages, > > > or at least not add it to even more messages. > > > > > If we instead think that errtable() is there just for programmatically > > > knowing the affected table, then we should add the table name to all > > > errmsg() where relevant, as in the patch under discussion. > > > > > What do others think? > > > > I believe that the intended use of errtable() and friends is so that > > applications don't have to parse those names out of a human-friendly > > message. We should add calls to them in cases where (a) an application > > can tell from the SQLSTATE that some particular table is involved > > and (b) it's likely that the app would wish to know which table that is. > > I don't feel a need to sprinkle every single ereport() in the backend > > with errtable(), just ones where there's a plausible use-case for the > > extra cycles that will be spent. > > > > On the other side of the coin, whether we use errtable() is not directly > > a factor in deciding what the human-friendly messages should say. > > I do find it hard to envision a case where we'd want to use errtable() > > and *not* put the table name in the error message, just because if > > applications need to know something then humans probably do too. > > > > makes sense. >
Thanks all for reviewing and giving comments. > > > Added relation name for this error. This can be verified by below > > > example: > > > Ex: > > > CREATE TABLE list_parted (a int,b char)PARTITION BY LIST (a); > > > CREATE TABLE part_1 (LIKE list_parted); > > > INSERT INTO part_1 VALUES (3, 'a'); > > > ALTER TABLE list_parted ATTACH PARTITION part_1 FOR VALUES IN (2); > > > > > > Without patch: > > > ERROR: partition constraint is violated by some row > > > With patch: > > > ERROR: partition constraint "part_1" is violated by some row > > > > Here it seems as if "part_1" is the constraint name. > > > > I agree. > > > It would be > > better to change it to: > > > > partition constraint is violated by some row in relation "part_1" OR > > partition constraint of relation "part_1" is violated b some row > > > > +1 for the second option suggested by Beena. I fixed above comment and updated expected .out files. Attaching updated patches. To make review simple, I made 3 patches as: v4_0001_rationalize_constraint_error_messages.patch: This patch has .c file changes. Added relation name in 6 error messages for check constraint. v4_0002_updated-regress-expected-.out-files.patch: This patch has changes of expected .out files for regress test suite. v4_0003_updated-constraints.source-file.patch: This patch has changes of .source file for constraints.sql regress test. Please review attached patches and let me know your comments. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
v4_0001_rationalize_constraint_error_messages.patch
Description: Binary data
v4_0002_updated-regress-expected-.out-files.patch
Description: Binary data
v4_0003_updated-constraints.source-file.patch
Description: Binary data
