Re: Error message inconsistency

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
---------------------------------------------------------------------------
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.

--
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com