Horton

Thread

Commits

  1. Added relation name in error messages for constraint checks.

  1. Error message inconsistency

    Simon Riggs <simon@2ndquadrant.com> — 2019-03-22T17:25:31Z

    As noted by a PostgreSQL user to me, error messages for NOT NULL
constraints are inconsistent - they do not mention the relation name in the
message, as all other variants of this message do. e.g.

postgres=# create table nn (id integer not null);
CREATE TABLE
postgres=# insert into nn values (NULL);
ERROR: null value in column "id" violates not-null constraint
DETAIL: Failing row contains (null).

postgres=# create table nn2 (id integer check (id is not null));
CREATE TABLE
postgres=# insert into nn2 values (NULL);
ERROR: new row for relation "nn2" violates check constraint "nn2_id_check"
DETAIL: Failing row contains (null).

I propose the attached patch as a fix, changing the wording (of the first
case) to
ERROR: null value in column "id" for relation "nn" violates not-null
constraint

It causes breakage in multiple tests, which is easy to fix once/if we agree
to change.

Thanks

-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  2. Re: Error message inconsistency

    Fabrízio de Royes Mello <fabriziomello@gmail.com> — 2019-03-22T23:03:20Z

    On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <simon@2ndquadrant.com> wrote:
>
> As noted by a PostgreSQL user to me, error messages for NOT NULL
constraints are inconsistent - they do not mention the relation name in the
message, as all other variants of this message do. e.g.
>
> postgres=# create table nn (id integer not null);
> CREATE TABLE
> postgres=# insert into nn values (NULL);
> ERROR: null value in column "id" violates not-null constraint
> DETAIL: Failing row contains (null).
>
> postgres=# create table nn2 (id integer check (id is not null));
> CREATE TABLE
> postgres=# insert into nn2 values (NULL);
> ERROR: new row for relation "nn2" violates check constraint "nn2_id_check"
> DETAIL: Failing row contains (null).
>
> I propose the attached patch as a fix, changing the wording (of the first
case) to
> ERROR: null value in column "id" for relation "nn" violates not-null
constraint
>
> It causes breakage in multiple tests, which is easy to fix once/if we
agree to change.
>

I totally agree with that change because I already get some negative
feedback from users about this message too.

Regards,

--
   Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

  3. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2019-03-24T13:02:18Z

    On Sat, Mar 23, 2019 at 4:33 AM Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <simon@2ndquadrant.com> wrote:
> >
> > As noted by a PostgreSQL user to me, error messages for NOT NULL constraints are inconsistent - they do not mention the relation name in the message, as all other variants of this message do. e.g.
> >
> > postgres=# create table nn (id integer not null);
> > CREATE TABLE
> > postgres=# insert into nn values (NULL);
> > ERROR: null value in column "id" violates not-null constraint
> > DETAIL: Failing row contains (null).
> >
> > postgres=# create table nn2 (id integer check (id is not null));
> > CREATE TABLE
> > postgres=# insert into nn2 values (NULL);
> > ERROR: new row for relation "nn2" violates check constraint "nn2_id_check"
> > DETAIL: Failing row contains (null).
> >
> > I propose the attached patch as a fix, changing the wording (of the first case) to
> > ERROR: null value in column "id" for relation "nn" violates not-null constraint
> >

I think we are inconsistent for a similar message at a few other
places as well.  See, below two messages:

column \"%s\" contains null values
column \"%s\" of table \"%s\" contains null values

If we decide to change this case, then why not change another place
which has a similar symptom?

> > It causes breakage in multiple tests, which is easy to fix once/if we agree to change.
> >
>
> I totally agree with that change because I already get some negative feedback from users about this message too.
>

What kind of negative feedback did you get from users?  If I see in
the log file, the message is displayed as :

2019-03-24 18:12:49.331 IST [6348] ERROR:  null value in column "id"
violates not-null constraint
2019-03-24 18:12:49.331 IST [6348] DETAIL:  Failing row contains (null).
2019-03-24 18:12:49.331 IST [6348] STATEMENT:  insert into nn values (NULL);

So, it is not difficult to identify the relation.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  4. Re: Error message inconsistency

    Greg Steiner <greg.steiner89@gmail.com> — 2019-03-24T13:41:01Z

    To me the error message that includes more detail is superior. Even though
you can get the detail from the logs, it seems like it would much more
convenient for it to be reported out via the error to allow
users/applications to identify the problem relation without fetching logs.
I understand if that's not worth breaking numerous tests, though.
Personally, I think consistency here is important enough to warrant it.

On Sun, Mar 24, 2019, 9:02 AM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Sat, Mar 23, 2019 at 4:33 AM Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> >
> > On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <simon@2ndquadrant.com>
> wrote:
> > >
> > > As noted by a PostgreSQL user to me, error messages for NOT NULL
> constraints are inconsistent - they do not mention the relation name in the
> message, as all other variants of this message do. e.g.
> > >
> > > postgres=# create table nn (id integer not null);
> > > CREATE TABLE
> > > postgres=# insert into nn values (NULL);
> > > ERROR: null value in column "id" violates not-null constraint
> > > DETAIL: Failing row contains (null).
> > >
> > > postgres=# create table nn2 (id integer check (id is not null));
> > > CREATE TABLE
> > > postgres=# insert into nn2 values (NULL);
> > > ERROR: new row for relation "nn2" violates check constraint
> "nn2_id_check"
> > > DETAIL: Failing row contains (null).
> > >
> > > I propose the attached patch as a fix, changing the wording (of the
> first case) to
> > > ERROR: null value in column "id" for relation "nn" violates not-null
> constraint
> > >
>
> I think we are inconsistent for a similar message at a few other
> places as well.  See, below two messages:
>
> column \"%s\" contains null values
> column \"%s\" of table \"%s\" contains null values
>
> If we decide to change this case, then why not change another place
> which has a similar symptom?
>
> > > It causes breakage in multiple tests, which is easy to fix once/if we
> agree to change.
> > >
> >
> > I totally agree with that change because I already get some negative
> feedback from users about this message too.
> >
>
> What kind of negative feedback did you get from users?  If I see in
> the log file, the message is displayed as :
>
> 2019-03-24 18:12:49.331 IST [6348] ERROR:  null value in column "id"
> violates not-null constraint
> 2019-03-24 18:12:49.331 IST [6348] DETAIL:  Failing row contains (null).
> 2019-03-24 18:12:49.331 IST [6348] STATEMENT:  insert into nn values
> (NULL);
>
> So, it is not difficult to identify the relation.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
>
>

  5. Re: Error message inconsistency

    Simon Riggs <simon@2ndquadrant.com> — 2019-03-24T18:23:17Z

    On Sun, 24 Mar 2019 at 13:02, Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Sat, Mar 23, 2019 at 4:33 AM Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> >
> > On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <simon@2ndquadrant.com>
> wrote:
> > >
> > > As noted by a PostgreSQL user to me, error messages for NOT NULL
> constraints are inconsistent - they do not mention the relation name in the
> message, as all other variants of this message do. e.g.
> > >
> > > postgres=# create table nn (id integer not null);
> > > CREATE TABLE
> > > postgres=# insert into nn values (NULL);
> > > ERROR: null value in column "id" violates not-null constraint
> > > DETAIL: Failing row contains (null).
> > >
> > > postgres=# create table nn2 (id integer check (id is not null));
> > > CREATE TABLE
> > > postgres=# insert into nn2 values (NULL);
> > > ERROR: new row for relation "nn2" violates check constraint
> "nn2_id_check"
> > > DETAIL: Failing row contains (null).
> > >
> > > I propose the attached patch as a fix, changing the wording (of the
> first case) to
> > > ERROR: null value in column "id" for relation "nn" violates not-null
> constraint
> > >
>
> I think we are inconsistent for a similar message at a few other
> places as well.  See, below two messages:
>
> column \"%s\" contains null values
> column \"%s\" of table \"%s\" contains null values
>
> If we decide to change this case, then why not change another place
> which has a similar symptom?
>

Yes, lets do that.

I'm passing on feedback, so if it applies in other cases, I'm happy to
change other common cases also for the benefit of users.

Do you have a list of cases you'd like to see changed?


> > > It causes breakage in multiple tests, which is easy to fix once/if we
> agree to change.
> > >
> >
> > I totally agree with that change because I already get some negative
> feedback from users about this message too.
> >
>
> What kind of negative feedback did you get from users?  If I see in
> the log file, the message is displayed as :
>
> 2019-03-24 18:12:49.331 IST [6348] ERROR:  null value in column "id"
> violates not-null constraint
> 2019-03-24 18:12:49.331 IST [6348] DETAIL:  Failing row contains (null).
> 2019-03-24 18:12:49.331 IST [6348] STATEMENT:  insert into nn values
> (NULL);
>
> So, it is not difficult to identify the relation.
>

The user is not shown the failing statement, and if they are, it might have
been generated for them.

Your example assumes the user has access to the log, that
log_min_error_statement is set appropriately and that the user can locate
their log entries to identify the table name. The log contains timed
entries but the user may not be aware of the time of the error accurately
enough to locate the correct statement amongst many others.

If the statement is modified by triggers or rules, then you have no chance.

e.g. add this to the above example:

create or replace rule rr as on insert to nn2 do instead insert into nn
values (new.*);


and its clear that the LOG of the statement, even if it is visible, is
misleading since the SQL refers to table nn, but the error is generated by
the insert into table nn2.


-- 
Simon Riggs                http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/>
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2019-03-25T03:15:19Z

    On Sun, Mar 24, 2019 at 11:53 PM Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On Sun, 24 Mar 2019 at 13:02, Amit Kapila <amit.kapila16@gmail.com> wrote:
>>
>> I think we are inconsistent for a similar message at a few other
>> places as well.  See, below two messages:
>>
>> column \"%s\" contains null values
>> column \"%s\" of table \"%s\" contains null values
>>
>> If we decide to change this case, then why not change another place
>> which has a similar symptom?
>
>
> Yes, lets do that.
>
> I'm passing on feedback, so if it applies in other cases, I'm happy to change other common cases also for the benefit of users.
>
> Do you have a list of cases you'd like to see changed?
>

I think we can once scrutinize all the error messages with error codes
ERRCODE_NOT_NULL_VIOLATION and ERRCODE_CHECK_VIOLATION to see if
anything else need change.

>>
>> > > It causes breakage in multiple tests, which is easy to fix once/if we agree to change.
>> > >
>> >
>> > I totally agree with that change because I already get some negative feedback from users about this message too.
>> >
>>
>> What kind of negative feedback did you get from users?  If I see in
>> the log file, the message is displayed as :
>>
>> 2019-03-24 18:12:49.331 IST [6348] ERROR:  null value in column "id"
>> violates not-null constraint
>> 2019-03-24 18:12:49.331 IST [6348] DETAIL:  Failing row contains (null).
>> 2019-03-24 18:12:49.331 IST [6348] STATEMENT:  insert into nn values (NULL);
>>
>> So, it is not difficult to identify the relation.
>
>
> The user is not shown the failing statement, and if they are, it might have been generated for them.
>

I can imagine that in some cases where queries/statements are
generated for some application, they might be presented just with
errors that occurred while execution and now it will be difficult to
identify the relation for which that problem has occurred.

> Your example assumes the user has access to the log, that log_min_error_statement is set appropriately and that the user can locate their log entries to identify the table name. The log contains timed entries but the user may not be aware of the time of the error accurately enough to locate the correct statement amongst many others.
>
> If the statement is modified by triggers or rules, then you have no chance.
>
> e.g. add this to the above example:
>
> create or replace rule rr as on insert to nn2 do instead insert into nn values (new.*);
>
>
> and its clear that the LOG of the statement, even if it is visible, is misleading since the SQL refers to table nn, but the error is generated by the insert into table nn2.
>

This example also indicates that it will be helpful for users to see
the relation name in the error message.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  7. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2019-03-25T03:23:27Z

    On Sun, Mar 24, 2019 at 7:11 PM Greg Steiner <greg.steiner89@gmail.com> wrote:
>
> To me the error message that includes more detail is superior. Even though you can get the detail from the logs, it seems like it would much more convenient for it to be reported out via the error to allow users/applications to identify the problem relation without fetching logs. I understand if that's not worth breaking numerous tests, though.
>

Yeah, I think that is the main point.  There will be a quite some
churn in the regression test output, but OTOH, if it is for good of
users, then it might be worth.

> Personally, I think consistency here is important enough to warrant it.
>

Fair point.  Can such an error message change break any application?
I see some cases where users have check based on Error Code, but not
sure if there are people who have check based on error messages.

Anyone else having an opinion on this matter?  Basically, I would like
to hear if anybody thinks that this change can cause any sort of
problem.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  8. Re: Error message inconsistency

    Robert Haas <robertmhaas@gmail.com> — 2019-03-25T16:11:33Z

    On Sun, Mar 24, 2019 at 11:23 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> Fair point.  Can such an error message change break any application?
> I see some cases where users have check based on Error Code, but not
> sure if there are people who have check based on error messages.

I'm sure there are -- in fact, I've written code that does that.  But
I also don't think that's a reason not to improve the error messages.
If we start worrying about stuff like this, we'll be unable to ever
improve anything.

> Anyone else having an opinion on this matter?  Basically, I would like
> to hear if anybody thinks that this change can cause any sort of
> problem.

I don't think it's going to cause a problem for users, provided the
patch is correct.  I wondered whether it was always going to pick up
the relation name, e.g. if partitioning is involved, but I didn't
check into it at all, so it may be fine.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  9. Re: Error message inconsistency

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-07-01T15:40:09Z

    Do we have an actual patch here?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  10. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2019-07-06T04:22:49Z

    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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  11. Re: Error message inconsistency

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-01-06T13:00:58Z

    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.


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

Please review attached patch and let me know feedback.

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

  12. Re: Error message inconsistency

    MBeena Emerson <mbeena.emerson@gmail.com> — 2020-01-09T12:12:19Z

    Hi Mahendra,

Thanks for the patch.
I am not sure but maybe the relation name should also be added to the
following test case?

create table t4 (id int);
insert into t4 values (1);
ALTER TABLE t4 ADD CONSTRAINT c1 CHECK (id > 10) NOT VALID; -- succeeds
ALTER TABLE t4 VALIDATE CONSTRAINT c1;
*ERROR:  check constraint "c1" is violated by some row*

On Mon, 6 Jan 2020 at 18:31, 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.
>
>
> *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.
>
> Please review attached patch and let me know feedback.
>
> Thanks and Regards
> Mahendra Singh Thalor
> EnterpriseDB: http://www.enterprisedb.com
>


-- 
--
M Beena Emerson

  13. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-21T05:19:46Z

    On Thu, Jan 9, 2020 at 5:42 PM MBeena Emerson <mbeena.emerson@gmail.com> wrote:
>

Hi Beena,

It is better to reply inline.

> Hi Mahendra,
>
> Thanks for the patch.
> I am not sure but maybe the relation name should also be added to the following test case?
>
> create table t4 (id int);
> insert into t4 values (1);
> ALTER TABLE t4 ADD CONSTRAINT c1 CHECK (id > 10) NOT VALID; -- succeeds
> ALTER TABLE t4 VALIDATE CONSTRAINT c1;
> ERROR:  check constraint "c1" is violated by some row
>

I see that in this case, we are using errtableconstraint which should
set table/schema name, but then that doesn't seem to be used.  Can we
explore it a bit from that angle?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  14. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-21T05:21:35Z

    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.

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

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  15. Re: Error message inconsistency

    MBeena Emerson <mbeena.emerson@gmail.com> — 2020-01-21T05:37:50Z

    Hi Amit,

On Tue, 21 Jan 2020 at 10:49, Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Thu, Jan 9, 2020 at 5:42 PM MBeena Emerson <mbeena.emerson@gmail.com>
> wrote:
> >
>
> Hi Beena,
>
> It is better to reply inline.
>
> > Hi Mahendra,
> >
> > Thanks for the patch.
> > I am not sure but maybe the relation name should also be added to the
> following test case?
> >
> > create table t4 (id int);
> > insert into t4 values (1);
> > ALTER TABLE t4 ADD CONSTRAINT c1 CHECK (id > 10) NOT VALID; -- succeeds
> > ALTER TABLE t4 VALIDATE CONSTRAINT c1;
> > ERROR:  check constraint "c1" is violated by some row
> >
>
> I see that in this case, we are using errtableconstraint which should
> set table/schema name, but then that doesn't seem to be used.  Can we
> explore it a bit from that angle?
>

The usage of the function errtableconstraint seems only to set the
schema_name table_name constraint_name internally and not for display
purposes. As seen in the following two cases where the relation name is
displayed using RelationGetRelationName and errtableconstraint is called as
part of errcode parameter not errmsg.

                  ereport(ERROR,
                    (errcode(ERRCODE_CHECK_VIOLATION),
                     errmsg("new row for relation \"%s\" violates check
constraint \"%s\"",
                            RelationGetRelationName(orig_rel), failed),
                     val_desc ? errdetail("Failing row contains %s.",
val_desc) : 0,
                     errtableconstraint(orig_rel, failed)));

                        ereport(ERROR,
                                (errcode(ERRCODE_UNIQUE_VIOLATION),
                                 errmsg("duplicate key value violates
unique constraint \"%s\"",
                                        RelationGetRelationName(rel)),
                                 key_desc ? errdetail("Key %s already
exists.",
                                                      key_desc) : 0,
                                 errtableconstraint(heapRel,

RelationGetRelationName(rel))));


--
M Beena Emerson

EnterpriseDB: http://www.enterprisedb.com

  16. Re: Error message inconsistency

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-01-21T14:39:47Z

    On 2020-Jan-21, MBeena Emerson wrote:

> The usage of the function errtableconstraint seems only to set the
> schema_name table_name constraint_name internally and not for display
> purposes. As seen in the following two cases where the relation name is
> displayed using RelationGetRelationName and errtableconstraint is called as
> part of errcode parameter not errmsg.

You can see those fields by raising the log verbosity; it's a
client-side thing. For example, in psql you can use
\set VERBOSITY verbose

In psql you can also use \errverbose after an error to print those
fields.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  17. Re: Error message inconsistency

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-01-22T07:39:58Z

    On Tue, 21 Jan 2020 at 20:09, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> On 2020-Jan-21, MBeena Emerson wrote:
>
> > The usage of the function errtableconstraint seems only to set the
> > schema_name table_name constraint_name internally and not for display
> > purposes. As seen in the following two cases where the relation name is
> > displayed using RelationGetRelationName and errtableconstraint is called as
> > part of errcode parameter not errmsg.
>
> You can see those fields by raising the log verbosity; it's a
> client-side thing. For example, in psql you can use
> \set VERBOSITY verbose
>
> In psql you can also use \errverbose after an error to print those
> fields.

Thanks for the explanation.

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

  18. Re: Error message inconsistency

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-01-22T07:55:55Z

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

  19. Re: Error message inconsistency

    MBeena Emerson <mbeena.emerson@gmail.com> — 2020-01-22T09:53:15Z

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

  20. Re: Error message inconsistency

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-01-22T13:32:49Z

    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?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  21. Re: Error message inconsistency

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-22T15:18:27Z

    Alvaro Herrera <alvherre@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.  But
saying that we can make the messages omit info because it's available
from these program-friendly fields seems 100% wrong to me, even if one
turns a blind eye to the fact that existing client code likely won't
show those fields to users.

			regards, tom lane

  22. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-23T04:31:19Z

    On Wed, Jan 22, 2020 at 3:23 PM MBeena Emerson <mbeena.emerson@gmail.com> wrote:
>
> > -----------------------------------------------------------------------------------------------------
> > 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.
>

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.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  23. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-23T04:50:19Z

    On Wed, Jan 22, 2020 at 8:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alvaro Herrera <alvherre@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.


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  24. Re: Error message inconsistency

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-01-23T12:21:00Z

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

  25. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-24T05:21:49Z

    On Thu, Jan 23, 2020 at 5:51 PM Mahendra Singh Thalor
<mahi6run@gmail.com> wrote:
>
> I fixed above comment and updated expected .out files. Attaching
> updated patches.
>

LGTM.  I have combined them into the single patch.  What do we think
about backpatching this?  As there are quite a few changes in the
regression tests, so it might be a good idea to keep the back branches
code in sync, but, OTOH, this is more of a change related to providing
more information, so we don't have any pressing need to backpatch
this.  What do others think?

One thing to note is that there are places in code where we use
'table' instead of 'relation' for the same thing in the error messages
as seen in the below places (the first one uses 'relation', the second
one uses 'table') and the patch is using 'relation' which I think is
fine.

1. 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));


2. 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)));

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  26. Re: Error message inconsistency

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-01-24T16:07:50Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> LGTM.  I have combined them into the single patch.  What do we think
> about backpatching this?

No objection to the patch for HEAD, but it does not seem like
back-patch material: it is not fixing a bug.

			regards, tom lane

  27. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-25T04:46:28Z

    On Fri, Jan 24, 2020 at 9:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > LGTM.  I have combined them into the single patch.  What do we think
> > about backpatching this?
>
> No objection to the patch for HEAD, but it does not seem like
> back-patch material: it is not fixing a bug.
>

Okay, I will commit this early next week (by Tuesday).

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  28. Re: Error message inconsistency

    Robert Haas <robertmhaas@gmail.com> — 2020-01-27T15:54:19Z

    On Fri, Jan 24, 2020 at 12:22 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> One thing to note is that there are places in code where we use
> 'table' instead of 'relation' for the same thing in the error messages
> as seen in the below places (the first one uses 'relation', the second
> one uses 'table') and the patch is using 'relation' which I think is
> fine.

We often use "relation" as a sort of a weasel-word when we don't know
the relkind; i.e. when we're complaining about something that might be
a view or index or foreign table or whatever. If we say "table," we
need to know that it is, precisely, a table.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  29. Re: Error message inconsistency

    Amit Kapila <amit.kapila16@gmail.com> — 2020-01-28T12:43:17Z

    On Sat, Jan 25, 2020 at 10:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jan 24, 2020 at 9:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > LGTM.  I have combined them into the single patch.  What do we think
> > > about backpatching this?
> >
> > No objection to the patch for HEAD, but it does not seem like
> > back-patch material: it is not fixing a bug.
> >
>
> Okay, I will commit this early next week (by Tuesday).
>

Pushed.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  30. Re: Error message inconsistency

    Mahendra Singh Thalor <mahi6run@gmail.com> — 2020-01-30T05:59:23Z

    On Tue, 28 Jan 2020 at 18:13, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sat, Jan 25, 2020 at 10:16 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Jan 24, 2020 at 9:37 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >
> > > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > > LGTM.  I have combined them into the single patch.  What do we think
> > > > about backpatching this?
> > >
> > > No objection to the patch for HEAD, but it does not seem like
> > > back-patch material: it is not fixing a bug.
> > >
> >
> > Okay, I will commit this early next week (by Tuesday).
> >
>
> Pushed.
>

Thank you for committing it!

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