Re: Error message inconsistency

On Thu, 23 Jan 2020 at 10:20, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jan 22, 2020 at 8:48 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > Alvaro Herrera <alvherre(at)2ndquadrant(dot)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