Re: Error message inconsistency

Hi Mahendra,

Thanks for working on this.

On Wed, 22 Jan 2020 at 13:26, Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
>
> On Tue, 21 Jan 2020 at 10:51, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Jan 6, 2020 at 6:31 PM Mahendra Singh Thalor <mahi6run(at)gmail(dot)com> wrote:
> > >
> > > On Sat, 6 Jul 2019 at 09:53, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > >
> > > > On Mon, Jul 1, 2019 at 10:05 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)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 above error 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 some row",
> 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 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 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 so attached patch is adding relation name with the "not-null constraint" error in 2 places. I didn't changed out files of test 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 with above 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