Error message inconsistency

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Error message inconsistency

Simon Riggs
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/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

rationalize_constraint_error_messages.v1.patch (992 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

fabriziomello


On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <[hidden email]> 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
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

akapila
On Sat, Mar 23, 2019 at 4:33 AM Fabrízio de Royes Mello
<[hidden email]> wrote:

>
> On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

Greg Steiner
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 <[hidden email]> wrote:
On Sat, Mar 23, 2019 at 4:33 AM Fabrízio de Royes Mello
<[hidden email]> wrote:
>
> On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

Simon Riggs
In reply to this post by akapila
On Sun, 24 Mar 2019 at 13:02, Amit Kapila <[hidden email]> wrote:
On Sat, Mar 23, 2019 at 4:33 AM Fabrízio de Royes Mello
<[hidden email]> wrote:
>
> On Fri, Mar 22, 2019 at 2:25 PM Simon Riggs <[hidden email]> 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/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

akapila
On Sun, Mar 24, 2019 at 11:53 PM Simon Riggs <[hidden email]> wrote:

>
> On Sun, 24 Mar 2019 at 13:02, Amit Kapila <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

akapila
In reply to this post by Greg Steiner
On Sun, Mar 24, 2019 at 7:11 PM Greg Steiner <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

Robert Haas
On Sun, Mar 24, 2019 at 11:23 PM Amit Kapila <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

Alvaro Herrera-9
In reply to this post by akapila
Do we have an actual patch here?

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


Reply | Threaded
Open this post in threaded view
|

Re: Error message inconsistency

akapila
On Mon, Jul 1, 2019 at 10:05 PM Alvaro Herrera <[hidden email]> 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