Re: Error message inconsistency

Hi Mahendra,

Thanks for working on this.

On Wed, 22 Jan 2020 at 13:26, Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
>
> On Tue, 21 Jan 2020 at 10:51, Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Jan 6, 2020 at 6:31 PM Mahendra Singh Thalor <mahi6run@gmail.com> wrote:
> > >
> > > On Sat, 6 Jul 2019 at 09:53, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 1, 2019 at 10:05 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > > >
> > > > > Do we have an actual patch here?
> > > > >
> > > >
> > > > We have a patch, but it needs some more work like finding similar
> > > > places and change all of them at the same time and then change the
> > > > tests to adapt the same.
> > > >
> > >
> > > Hi all,
> > > Based on above discussion, I tried to find out all the places where we need to change error for "not null
constraint". As Amit Kapila pointed out 1 place, I changed the error and adding modified patch. 
> > >
> >
> > It seems you have not used the two error codes
> > (ERRCODE_NOT_NULL_VIOLATION and ERRCODE_CHECK_VIOLATION) pointed by me
> > above.
>
> Thanks Amit and Beena for reviewing patch.
>
> Yes, you are correct. I searched using error messages not error code.  That was my mistake.  Now, I grepped using
aboveerror codes and found that these error codes are used in 19 places. Below is the code parts of 19 places. 
>
> 1. src/backend/utils/adt/domains.c
>
> 146                 if (isnull)
> 147                     ereport(ERROR,
> 148                             (errcode(ERRCODE_NOT_NULL_VIOLATION),
> 149                              errmsg("domain %s does not allow null values",
> 150                                     format_type_be(my_extra->domain_type)),
> 151                              errdatatype(my_extra->domain_type)));
> 152                 break;
>
> I think, above error is for domain, so there is no need to add anything in error message.
> -----------------------------------------------------------------------------------------------------
> 2. src/backend/utils/adt/domains.c
>
> 181                     if (!ExecCheck(con->check_exprstate, econtext))
> 182                         ereport(ERROR,
> 183                                 (errcode(ERRCODE_CHECK_VIOLATION),
> 184                                  errmsg("value for domain %s violates check constraint \"%s\"",
> 185                                         format_type_be(my_extra->domain_type),
> 186                                         con->name),
> 187                                  errdomainconstraint(my_extra->domain_type,
> 188                                                      con->name)));
>
> I think, above error is for domain, so there is no need to add anything in error message.
> -----------------------------------------------------------------------------------------------------
> 3. src/backend/partitioning/partbounds.c
>
> 1330             if (part_rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
> 1331                 ereport(WARNING,
> 1332                         (errcode(ERRCODE_CHECK_VIOLATION),
> 1333                          errmsg("skipped scanning foreign table \"%s\" which is a partition of default partition
\"%s\"",
> 1334                                 RelationGetRelationName(part_rel),
> 1335                                 RelationGetRelationName(default_rel))));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 4. src/backend/partitioning/partbounds.c
>
> 1363             if (!ExecCheck(partqualstate, econtext))
> 1364                 ereport(ERROR,
> 1365                         (errcode(ERRCODE_CHECK_VIOLATION),
> 1366                          errmsg("updated partition constraint for default partition \"%s\" would be violated by
somerow", 
> 1367                                 RelationGetRelationName(default_rel))));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 5. src/backend/executor/execPartition.c
>
> 342             ereport(ERROR,
>  343                     (errcode(ERRCODE_CHECK_VIOLATION),
>  344                      errmsg("no partition of relation \"%s\" found for row",
>  345                             RelationGetRelationName(rel)),
>  346                      val_desc ?
>  347                      errdetail("Partition key of the failing row contains %s.",
>  348                                val_desc) : 0));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 6. src/backend/executor/execMain.c
>
> 1877     ereport(ERROR,
> 1878             (errcode(ERRCODE_CHECK_VIOLATION),
> 1879              errmsg("new row for relation \"%s\" violates partition constraint",
> 1880                     RelationGetRelationName(resultRelInfo->ri_RelationDesc)),
> 1881              val_desc ? errdetail("Failing row contains %s.", val_desc) : 0));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 7. src/backend/executor/execMain.c
>
> 1958                 ereport(ERROR,
> 1959                         (errcode(ERRCODE_NOT_NULL_VIOLATION),
> 1960                          errmsg("null value in column \"%s\" violates not-null constraint",
> 1961                                 NameStr(att->attname)),
> 1962                          val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
> 1963                          errtablecol(orig_rel, attrChk)));
>
> Added relation name for this error.  This can be verified by below example:
> Ex:
> CREATE TABLE test (a int PRIMARY KEY, b int GENERATED ALWAYS AS (nullif(a, 0)) STORED NOT NULL);
> INSERT INTO test (a) VALUES (1);
> INSERT INTO test (a) VALUES (0);
>
> Without patch:
> ERROR:  null value in column "b" violates not-null constraint
> DETAIL:  Failing row contains (0, null).
> With patch:
> ERROR:  null value in column "b" of relation "test" violates not-null constraint
> DETAIL:  Failing row contains (0, null).
> -----------------------------------------------------------------------------------------------------
> 8. src/backend/executor/execMain.c
>
> 2006             ereport(ERROR,
> 2007                     (errcode(ERRCODE_CHECK_VIOLATION),
> 2008                      errmsg("new row for relation \"%s\" violates check constraint \"%s\"",
> 2009                             RelationGetRelationName(orig_rel), failed),
> 2010                      val_desc ? errdetail("Failing row contains %s.", val_desc) : 0,
> 2011                      errtableconstraint(orig_rel, failed)));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 9. src/backend/executor/execExprInterp.c
>
> 3600         ereport(ERROR,
> 3601                 (errcode(ERRCODE_NOT_NULL_VIOLATION),
> 3602                  errmsg("domain %s does not allow null values",
> 3603                         format_type_be(op->d.domaincheck.resulttype)),
> 3604                  errdatatype(op->d.domaincheck.resulttype)));
>
> I think, above error is for domain, so there is no need to add anything in error message.
> -----------------------------------------------------------------------------------------------------
> 10. src/backend/executor/execExprInterp.c
>
> 3615         ereport(ERROR,
> 3616                 (errcode(ERRCODE_CHECK_VIOLATION),
> 3617                  errmsg("value for domain %s violates check constraint \"%s\"",
> 3618                         format_type_be(op->d.domaincheck.resulttype),
> 3619                         op->d.domaincheck.constraintname),
> 3620                  errdomainconstraint(op->d.domaincheck.resulttype,
> 3621                                      op->d.domaincheck.constraintname)));
>
> I think, above error is for domain, so there is no need to add anything in error message.
> -----------------------------------------------------------------------------------------------------
> 11. src/backend/commands/tablecmds.c
>
> 5273                     ereport(ERROR,
>  5274                             (errcode(ERRCODE_NOT_NULL_VIOLATION),
>  5275                              errmsg("column \"%s\" contains null values",
>  5276                                     NameStr(attr->attname)),
>  5277                              errtablecol(oldrel, attn + 1)));
>
> Added relation name for this error.  This can be verified by below example:
> Ex:
> CREATE TABLE test (a int);
> INSERT INTO test VALUES (0), (1);
> ALTER TABLE test ADD COLUMN b int NOT NULL, ALTER COLUMN b ADD GENERATED ALWAYS AS IDENTITY;
>
> Without patch:
> ERROR:  column "b" contains null values
> With patch:
> ERROR:  column "b" of relation "test" contains null values
> -----------------------------------------------------------------------------------------------------
> 12. src/backend/commands/tablecmds.c
>
>  5288                         if (!ExecCheck(con->qualstate, econtext))
>  5289                             ereport(ERROR,
>  5290                                     (errcode(ERRCODE_CHECK_VIOLATION),
>  5291                                      errmsg("check constraint \"%s\" is violated by some row",
>  5292                                             con->name),
>  5293                                      errtableconstraint(oldrel, con->name)));
>
> Added relation name for this error.  This can be verified by below example:
> Ex:
> CREATE TABLE test (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) STORED);
> INSERT INTO test (a) VALUES (10), (30);
> ALTER TABLE test ADD CHECK (b < 50);
>
> Without patch:
> ERROR:  check constraint "test_b_check" is violated by some row
> With patch:
> ERROR:  check constraint "test_b_check" of relation "test" is violated by some row
> -----------------------------------------------------------------------------------------------------
> 13. src/backend/commands/tablecmds.c
>
>  5306                 if (tab->validate_default)
>  5307                     ereport(ERROR,
>  5308                             (errcode(ERRCODE_CHECK_VIOLATION),
>  5309                              errmsg("updated partition constraint for default partition would be violated by
somerow"))); 
>
> 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 list_parted_def PARTITION OF list_parted DEFAULT;
> INSERT INTO list_parted_def VALUES (11, 'z');
> CREATE TABLE part_1 (LIKE list_parted);
> ALTER TABLE list_parted ATTACH PARTITION part_1 FOR VALUES IN (11);
>
> Without patch:
> ERROR:  updated partition constraint for default partition would be violated by some row
> With patch:
> ERROR:  updated partition constraint for default partition "list_parted_def" would be violated by some row
> -----------------------------------------------------------------------------------------------------
> 14. src/backend/commands/tablecmds.c
>
>  5310                 else
>  5311                     ereport(ERROR,
>  5312                             (errcode(ERRCODE_CHECK_VIOLATION),
>  5313                              errmsg("partition constraint is violated by some row")));
>
> 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. 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


> ---------------------------------------------------------------------------
> 15. src/backend/commands/tablecmds.c
>
> 10141             ereport(ERROR,
> 10142                     (errcode(ERRCODE_CHECK_VIOLATION),
> 10143                      errmsg("check constraint \"%s\" is violated by some row",
> 10144                             NameStr(constrForm->conname)),
> 10145                      errtableconstraint(rel, NameStr(constrForm->conname))));
>
> Added relation name for this error.  This can be verified by below example:
> Ex:
> CREATE TABLE test (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) STORED);
> INSERT INTO test (a) VALUES (10), (30);
> ALTER TABLE test ADD CONSTRAINT chk CHECK (b < 50) NOT VALID;
> ALTER TABLE test VALIDATE CONSTRAINT chk;
>
> Without patch:
> ERROR:  check constraint "chk" is violated by some row
> With patch:
> ERROR:  check constraint "chk" of relation "test" is violated by some row
> -----------------------------------------------------------------------------------------------------
> 16. src/backend/commands/typecmds.c
>
> 2396                         ereport(ERROR,
> 2397                                 (errcode(ERRCODE_NOT_NULL_VIOLATION),
> 2398                                  errmsg("column \"%s\" of table \"%s\" contains null values",
> 2399                                         NameStr(attr->attname),
> 2400                                         RelationGetRelationName(testrel)),
> 2401                                  errtablecol(testrel, attnum)));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 17. src/backend/commands/typecmds.c
>
> 2824                     ereport(ERROR,
> 2825                             (errcode(ERRCODE_CHECK_VIOLATION),
> 2826                              errmsg("column \"%s\" of table \"%s\" contains values that violate the new
constraint",
> 2827                                     NameStr(attr->attname),
> 2828                                     RelationGetRelationName(testrel)),
> 2829                              errtablecol(testrel, attnum)));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 18. src/backend/commands/typecmds.c
>
> 2396                         ereport(ERROR,
> 2397                                 (errcode(ERRCODE_NOT_NULL_VIOLATION),
> 2398                                  errmsg("column \"%s\" of table \"%s\" contains null values",
> 2399                                         NameStr(attr->attname),
> 2400                                         RelationGetRelationName(testrel)),
> 2401                                  errtablecol(testrel, attnum)));
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> 19. src/backend/commands/typecmds.c
>
> 2824                     ereport(ERROR,
> 2825                             (errcode(ERRCODE_CHECK_VIOLATION),
> 2826                              errmsg("column \"%s\" of table \"%s\" contains values that violate the new
constraint",
> 2827                                     NameStr(attr->attname),
> 2828                                     RelationGetRelationName(testrel)),
> 2829                              errtablecol(testrel, attnum)))
>
> Relation name is already appended in error messgae.
> -----------------------------------------------------------------------------------------------------
> >
> > > What does this patch?
> > > Before this patch, to display error of "not-null constraint", we were not displaying relation name in some cases
soattached patch is adding relation name with the "not-null constraint" error in 2 places. I didn't changed out files
oftest suite as we haven't finalized error messages. 
> > >
> > > I verified Robert's point of for partition tables also. With the error, we are adding relation name of "child
table"and i think, it is correct. 
> > >
> >
> > Can you show the same with the help of an example?
>
> Okay.  Below is the example:
> create table parent (a int, b int not null) partition by range (a);
> create table ch1 partition of parent for values from ( 10 ) to (20);
> postgres=# insert into parent values (9);
> ERROR:  no partition of relation "parent" found for row
> DETAIL:  Partition key of the failing row contains (a) = (9).
> postgres=# insert into parent values (11);
> ERROR:  null value in column "b" of relation "ch1" violates not-null constraint
> DETAIL:  Failing row contains (11, null).
>
> Attaching a patch for review.  In this patch, total 6 places I added relation name in error message and verifyed same
withabove mentioned examples. 
>
> Please review attahced patch and let me know your feedback.  I haven't modifed .out files because we haven't finalied
patch.
>




--

M Beena Emerson

EnterpriseDB: http://www.enterprisedb.com