patch for 9.2: enhanced errors

Hello

Attached patch implements a new erros's fields that describes table,
colums related to error. This enhanced info is limited to constraints
and RI.

example:

postgres=# create table omega(a int unique not null check (a > 10));
NOTICE: 00000: CREATE TABLE / UNIQUE will create implicit index
"omega_a_key" for table "omega"
LOCATION: DefineIndex, indexcmds.c:389
CREATE TABLE
Time: 106.867 ms
postgres=# \set VERBOSITY verbose
postgres=# insert into omega values(0);
ERROR: 23514: new row for relation "omega" violates check constraint
"omega_a_check"
LOCATION: ExecConstraints, execMain.c:1547
CONSTRAINT: omega_a_check
SCHEMA: public
TABLE: omega
COLUMNS: a
postgres=# insert into omega values(null);
ERROR: 23502: null value in column "a" violates not-null constraint
LOCATION: ExecConstraints, execMain.c:1519
CONSTRAINT: not_null_constraint
SCHEMA: public
TABLE: omega
COLUMNS: a
postgres=# insert into omega values(20);
INSERT 0 1
Time: 60.588 ms
postgres=# insert into omega values(20);
ERROR: 23505: duplicate key value violates unique constraint "omega_a_key"
DETAIL: Key (a)=(20) already exists.
LOCATION: _bt_check_unique, nbtinsert.c:432
CONSTRAINT: omega_a_key
SCHEMA: public
TABLE: omega
COLUMNS: a
postgres=#

This is base for support variables CONSTRAINT_NAME, SCHEMA_NAME and
TABLE_NAME for GET DIAGNOSTICS statement.

All regress tests was successfully passed

Regards

Pavel Stehule

On 11-06-08 04:14 PM, Pavel Stehule wrote:
> Hello
>
> Attached patch implements a new erros's fields that describes table,
> colums related to error. This enhanced info is limited to constraints
> and RI.
>

Here is my review of this patch

Submission Review:
------------------------
The patch applies cleanly against master
The patch does not include any documentation updates (see note below to
update config.sgml)
The patch does not include any unit tests. At a minimum it should add a
few tests with verbosity set to verbose

Usability Review
--------------------
The patch adds the ability to get more information about the reasons a
query failed. Pavel indicates that this is a building block for a later
patch. This sounds like a worthwhile goal, without this patch I don't
see another good way of getting the details on what columns make up the
constraint that fails, other than making additional queries into the
catalog.

This patch should not impact pg_dump or pg_upgrade.

Pavel has submitted a related patch that adds support for this feature
to plpgsql, in theory other pl's might want to use the information this
patch exposes.

Feature Test
-------------------

The error messages behave as described with \set verbosity verbose.

I tried this using both the 8.3 and 9.0 versions of psql (against a
postgresql server with this patch) and things worked as expected (the
extra error messages did not show). I also tried the patched psql
against an 8.3 backend and verified that we don't get strange behaviour
going against an older backend with verbosity set.

I tried adding multiple constraints to a table and inserting a row that
violates them, only one of the constraints showed up in the error
message, this is fine and consistent with the existing behaviour

Consider this example of an error that gets generated

ERROR: 23505: duplicate key value violates unique constraint "A_pkey"
DETAIL: Key (a)=(1) already exists.
LOCATION: _bt_check_unique, nbtinsert.c:433
CONSTRAINT: A_pkey
SCHEMA: public
TABLE: A
COLUMN: a
STATEMENT: insert into "A" values (1);

I think that both the CONSTRAINT, and TABLE name should be double quoted
like "A_pkey" is above. When doing this make sure you don't break the
quoting in the CSV format log.

Performance Review
-----------------------------
I don't see this patch impacting performance, I did not conduct any
performance tests.

Coding Review
-----------------

In tupdesc.c

line 202 the existing code is performing a deep copy of ConstrCheck. Do
you need to copy nkeys and conkey here as well?

Then at line 250 ccname is freed but not conkey

postgres_ext.h line 55
+ #define PG_DIAG_SCHEMA_NAME 's'
+ #define PG_DIAG_TABLE_NAME 't'
+ #define PG_DIAG_COLUMN_NAMES 'c'
+ #define PG_DIAG_CONSTRAINT_NAME 'n'

The assignment of letters to parameters seems arbitrary to me, I don't
have a different non-arbitrary mapping in mind but if anyone else does
they should speak up. I think it will be difficult to change this after
9.2 goes out.

elog.c:
***************
*** 2197,2202 ****
--- 2299,2319 ----
if (application_name)
appendCSVLiteral(&buf, application_name);

+ /* constraint_name */
+ appendCSVLiteral(&buf, edata->constraint_name);
+ appendStringInfoChar(&buf, ',');
+
+ /* schema name */
+ appendCSVLiteral(&buf, edata->schema_name);
+ appendStringInfoChar(&buf, ',');

You need to update config.sgml at the same time you update this format.
You need to append a "," after application name but before
constraintName. As it stands the CSV log has something like:
.....nbtinsert.c:433","psql""a_pkey","public","a","a"

nbtinsert.c

pg_get_indrelation is named differently than everything else in this
file (ie _bt...). My guess is that this function belongs somewhere else
but I don't know the code well enough to say where you should move it too.

Everything I've mentioned above is a minor issue, I will move the patch
to 'waiting for author' and wait for you to release an updated patch.

Steve Singer

On 11-06-18 06:36 PM, Steve Singer wrote:
> On 11-06-08 04:14 PM, Pavel Stehule wrote:
>
> Here is my review of this patch
>
> Submission Review:
> ------------------------
> The patch applies cleanly against master
> The patch does not include any documentation updates (see note below
> to update config.sgml)
> The patch does not include any unit tests. At a minimum it should add
> a few tests with verbosity set to verbose
>

On second thought tests might not work. Is the only way to get the
constraint details are in verbose mode where line numbers from the c
file are also included? If so then this won't work for the regression
tests. Having the diff comparison fail every time someone makes an
unrelated change to a source file isn't what we want.

Hello

2011/6/19 Steve Singer <ssinger_pg(at)sympatico(dot)ca>:
> On 11-06-08 04:14 PM, Pavel Stehule wrote:
>>
>> Hello
>>
>> Attached patch implements a new erros's fields that describes table,
>> colums related to error. This enhanced info is limited to constraints
>> and RI.
>>
>
...
>
> I think that both the CONSTRAINT, and TABLE name should be double quoted
> like "A_pkey" is above.  When doing this make sure you don't break the
> quoting in the CSV format log.
>

I agree so quoting must be used in CSV log - the result have to be
valid CSV and I'll ensure this. I am not sure about implicit quoting
and using some quote_ident operation early. This is result of some
operation - not input. Quoting in message is used not like SQL
quoting, but as plain text quoting - it is just border between human
readable text and data. But fields like TABLE_NAME or COLUMN_NAME
contains just data - so quoting is useless.

Next argument - the quoting is more simple than remove quoting. If
somebody needs to quoting, then can use a quoting_ident function, but
there are no inverse function - so I prefer a names in raw format. It
is more simply and usual to add quoting than remove quoting.

What do you think about?

>
> Performance Review
> -----------------------------
> I don't see this patch impacting performance, I did not conduct any
> performance tests.
>
>
> Coding Review
> -----------------
>
>
> In tupdesc.c
>
> line 202 the existing code is performing a deep copy of ConstrCheck.  Do you
> need to copy nkeys and conkey here as well?
>
> Then at line 250 ccname is freed but not conkey
>

I have to look on this

>
> postgres_ext.h line 55
> + #define PG_DIAG_SCHEMA_NAME    's'
> + #define PG_DIAG_TABLE_NAME    't'
> + #define PG_DIAG_COLUMN_NAMES    'c'
> + #define PG_DIAG_CONSTRAINT_NAME 'n'
>
> The assignment of letters to parameters seems arbitrary to me, I don't have
> a different non-arbitrary mapping in mind but if anyone else does they
> should speak up.  I think it will be difficult to change this after 9.2 goes
> out.
>
>
> elog.c:
> ***************
> *** 2197,2202 ****
> --- 2299,2319 ----
>      if (application_name)
>          appendCSVLiteral(&buf, application_name);
>
> +     /* constraint_name */
> +     appendCSVLiteral(&buf, edata->constraint_name);
> +     appendStringInfoChar(&buf, ',');
> +
> +     /* schema name */
> +     appendCSVLiteral(&buf, edata->schema_name);
> +     appendStringInfoChar(&buf, ',');
>
> You need to update config.sgml at the same time you update this format.
> You need to append a "," after application name but before constraintName.
> As it stands the CSV log has something like:
> .....nbtinsert.c:433","psql""a_pkey","public","a","a"
>

ok

>
> nbtinsert.c
>
> pg_get_indrelation is named differently than everything else in this file
> (ie _bt...).  My guess is that this function belongs somewhere else but I
> don't know the code well enough to say where you should move it too.
>

I'll try to get better name, but I would not use a public name like _bt

>
>
> Everything I've mentioned above is a minor issue, I will move the patch to
> 'waiting for author' and wait for you to release an updated patch.
>
> Steve Singer
>

ok

Thank you very much

Pavel Stehule

2011/6/19 Steve Singer <ssinger_pg(at)sympatico(dot)ca>:
> On 11-06-18 06:36 PM, Steve Singer wrote:
>>
>> On 11-06-08 04:14 PM, Pavel Stehule wrote:
>>
>> Here is my review of this patch
>>
>> Submission Review:
>> ------------------------
>> The patch applies cleanly against master
>> The patch does not include any documentation updates (see note below to
>> update config.sgml)
>> The patch does not include any unit tests. At a minimum it should add a
>> few tests with verbosity set to verbose
>>
>
> On second thought tests might not work. Is the only way to get the
> constraint details are in verbose mode where line numbers from the c file
> are also included? If so then this won't work for the regression tests.
> Having the diff comparison fail every time someone makes an unrelated change
> to a source file isn't what we want.
>

it is reason why patch doesn't any regress test changes. I have to
look, if verbose mode is documented somewhere.

Regards

Pavel Stehule

>

On Jun19, 2011, at 05:10 , Steve Singer wrote:
> On 11-06-18 06:36 PM, Steve Singer wrote:
>> On 11-06-08 04:14 PM, Pavel Stehule wrote:
>>
>> Here is my review of this patch
>>
>> Submission Review:
>> ------------------------
>> The patch applies cleanly against master
>> The patch does not include any documentation updates (see note below to update config.sgml)
>> The patch does not include any unit tests. At a minimum it should add a few tests with verbosity set to verbose
>>
>
> On second thought tests might not work. Is the only way to get the constraint details are in verbose mode where line numbers from the c file are also included? If so then this won't work for the regression tests. Having the diff comparison fail every time someone makes an unrelated change to a source file isn't what we want.

Speaking as someone who's wished for the feature that Pavel's patch provides
many times in the past - shouldn't there also be a field containing the
offending value? If we had that, it'd finally be possible to translate
constraint-related error messages to informative messages for the user.

best regards,
Florian Pflug

2011/6/19 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jun19, 2011, at 05:10 , Steve Singer wrote:
>> On 11-06-18 06:36 PM, Steve Singer wrote:
>>> On 11-06-08 04:14 PM, Pavel Stehule wrote:
>>>
>>> Here is my review of this patch
>>>
>>> Submission Review:
>>> ------------------------
>>> The patch applies cleanly against master
>>> The patch does not include any documentation updates (see note below to update config.sgml)
>>> The patch does not include any unit tests. At a minimum it should add a few tests with verbosity set to verbose
>>>
>>
>> On second thought tests might not work. Is the only way to get the constraint details are in verbose mode where line numbers from the c file are also included? If so then this won't work for the regression tests.   Having the diff comparison fail every time someone makes an unrelated change to a source file isn't what we want.
>
> Speaking as someone who's wished for the feature that Pavel's patch provides
> many times in the past - shouldn't there also be a field containing the
> offending value? If we had that, it'd finally be possible to translate
> constraint-related error messages to informative messages for the user.

The value is available in almost cases. There is only one issue - this
should not be only one value - it could be list of values - so basic
question is about format and property name. PostgreSQL doesn't hold
relation between column and column constraint - all column constraints
are transformed to table constrains. All column informations are
derived from constraint - so when constraint is a > b and this
constraint is false, we have two values.

Maybe there is second issue (little bit - performance - you have to
call a output function), But I agree, so this information is very
interesting and can help.

I am open for any ideas in this direction.

Regards

Pavel

>
> best regards,
> Florian Pflug
>
>

On Jun19, 2011, at 14:03 , Pavel Stehule wrote:
> 2011/6/19 Florian Pflug <fgp(at)phlo(dot)org>:
>> Speaking as someone who's wished for the feature that Pavel's patch provides
>> many times in the past - shouldn't there also be a field containing the
>> offending value? If we had that, it'd finally be possible to translate
>> constraint-related error messages to informative messages for the user.
>
> The value is available in almost cases. There is only one issue - this
> should not be only one value - it could be list of values - so basic
> question is about format and property name. PostgreSQL doesn't hold
> relation between column and column constraint - all column constraints
> are transformed to table constrains. All column informations are
> derived from constraint - so when constraint is a > b and this
> constraint is false, we have two values.

Hm, you could rename COLUMN to VALUE, make it include the value,
and repeat it for every column in the constraint or index that caused
the error. For example, you'd get

VALUE: "a":5
VALUE: "b":3

if you violated a CHECK constraint asserting that "a < b".

You could also use that in custom constraint enforcement triggers -
i.e. I'm maintaining an application that enforces foreign key
constraints for arrays. With VALUE fields available, I could emit
one value field for every offending array member.

If repeating the same field multiple times is undesirable, the
information could of course be packed into one field, giving

VALUES: ("a":5, "b":3)

for the example from above. My array FK constraint trigger would
the presumably report

VALUES: ("array_field":42, "array_field":23)

if array members 42 and 23 lacked a corresponding row in the
referenced table.

That'd also work work for foreign keys and unique constraints. Exclusion
constraints are harder, because there the conflicting value might also
be of interest. (Hm, actually it might even be for unique indices if
some columns are NULL - not sure right now if there's a mode where we
treat NULL as a kind of wildcard...).

best regards,
Florian Pflug

2011/6/19 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jun19, 2011, at 14:03 , Pavel Stehule wrote:
>> 2011/6/19 Florian Pflug <fgp(at)phlo(dot)org>:
>>> Speaking as someone who's wished for the feature that Pavel's patch provides
>>> many times in the past - shouldn't there also be a field containing the
>>> offending value? If we had that, it'd finally be possible to translate
>>> constraint-related error messages to informative messages for the user.
>>
>> The value is available in almost cases. There is only one issue - this
>> should not be only one value - it could be list of values - so basic
>> question is about format and property name. PostgreSQL doesn't hold
>> relation between column and column constraint - all column constraints
>> are transformed to table constrains. All column informations are
>> derived from constraint - so when constraint is a > b and this
>> constraint is false, we have two values.
>
> Hm, you could rename COLUMN to VALUE, make it include the value,
> and repeat it for every column in the constraint or index that caused
> the error. For example, you'd get
>
> VALUE: "a":5
> VALUE: "b":3
>

I don't have a idea. These data should be available via GET
DIAGNOSTICS statement, so you can't use a repeated properties. I would
to use a simple access to column names because it is in ANSI SQL.

> if you violated a CHECK constraint asserting that "a < b".
>
> You could also use that in custom constraint enforcement triggers -
> i.e. I'm maintaining an application that enforces foreign key
> constraints for arrays. With VALUE fields available, I could emit
> one value field for every offending array member.
>
> If repeating the same field multiple times is undesirable, the
> information could of course be packed into one field, giving
>
> VALUES: ("a":5, "b":3)
>
> for the example from above. My array FK constraint trigger would
> the presumably report
>
> VALUES: ("array_field":42, "array_field":23)
>

there should be some similar, but probably we need to have some
dictionary type in core before. If we are too hurry, then we can have
a problem with backing compatibility :(. Theoretically we have a know
columns in COLUMNS property, so we can serialize values in same order
in serialized array format.

COLUMNS: a, b, c
VALUES: some, else, "some with \" or , "

Regards

Pavel

> if array members 42 and 23 lacked a corresponding row in the
> referenced table.
>
> That'd also work work for foreign keys and unique constraints. Exclusion
> constraints are harder, because there the conflicting value might also
> be of interest. (Hm, actually it might even be for unique indices if
> some columns are NULL - not sure right now if there's a mode where we
> treat NULL as a kind of wildcard...).
>
> best regards,
> Florian Pflug
>
>

On Sun, 19 Jun 2011, Pavel Stehule wrote:

> Maybe there is second issue (little bit - performance - you have to
> call a output function), But I agree, so this information is very
> interesting and can help.

I am concerned about the performance impact of doing that. Not all
constraints are on int4 columns. Some constraints might be on a geometry
type that is megabytes in side taking a substantial chunk of CPU and
bandwith to convert it into a text representation and then send it back to
the client.

> I am open for any ideas in this direction.
>
> Regards
>
> Pavel
>
>>
>> best regards,
>> Florian Pflug
>>
>>
>

Excerpts from Pavel Stehule's message of dom jun 19 06:51:13 -0400 2011:
> Hello
>
> 2011/6/19 Steve Singer <ssinger_pg(at)sympatico(dot)ca>:
> > On 11-06-08 04:14 PM, Pavel Stehule wrote:

> > nbtinsert.c
> >
> > pg_get_indrelation is named differently than everything else in this file
> > (ie _bt...).  My guess is that this function belongs somewhere else but I
> > don't know the code well enough to say where you should move it too.
> >
>
> I'll try to get better name, but I would not use a public name like _bt

lsyscache.c?

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

I am sending a updated patch

>
> Coding Review
> -----------------
>
>
> In tupdesc.c
>
> line 202 the existing code is performing a deep copy of ConstrCheck.  Do you
> need to copy nkeys and conkey here as well?
>
> Then at line 250 ccname is freed but not conkey
>

fixed

>
> postgres_ext.h line 55
> + #define PG_DIAG_SCHEMA_NAME    's'
> + #define PG_DIAG_TABLE_NAME    't'
> + #define PG_DIAG_COLUMN_NAMES    'c'
> + #define PG_DIAG_CONSTRAINT_NAME 'n'
>
> The assignment of letters to parameters seems arbitrary to me, I don't have
> a different non-arbitrary mapping in mind but if anyone else does they
> should speak up.  I think it will be difficult to change this after 9.2 goes
> out.
>
>
> elog.c:
> ***************
> *** 2197,2202 ****
> --- 2299,2319 ----
>      if (application_name)
>          appendCSVLiteral(&buf, application_name);
>
> +     /* constraint_name */
> +     appendCSVLiteral(&buf, edata->constraint_name);
> +     appendStringInfoChar(&buf, ',');
> +
> +     /* schema name */
> +     appendCSVLiteral(&buf, edata->schema_name);
> +     appendStringInfoChar(&buf, ',');
>
> You need to update config.sgml at the same time you update this format.
> You need to append a "," after application name but before constraintName.
> As it stands the CSV log has something like:
> .....nbtinsert.c:433","psql""a_pkey","public","a","a"

fixed

>
>
> nbtinsert.c
>
> pg_get_indrelation is named differently than everything else in this file
> (ie _bt...).  My guess is that this function belongs somewhere else but I
> don't know the code well enough to say where you should move it too.
>

I renamed this function to IndexRelationGetParentRelation and muved to
relcache.c

I don't call a quote_identifier on only data error properties like
table_name or schema_name (but I am open to arguments for it or
against it). The quote_identifier is used for column names, because
there should be a more names and comma should be used inside name -
and this is consistent with pg_get_indexdef_columns.

Regards

Pavel Stehule

On 11-06-20 03:44 PM, Pavel Stehule wrote:
> Hello
>>
>> You need to update config.sgml at the same time you update this format.
>> You need to append a "," after application name but before constraintName.
>> As it stands the CSV log has something like:
>> .....nbtinsert.c:433","psql""a_pkey","public","a","a"
> fixed
>

The CSV log seems fine now.

>>
>> nbtinsert.c
>>
>> pg_get_indrelation is named differently than everything else in this file
>> (ie _bt...). My guess is that this function belongs somewhere else but I
>> don't know the code well enough to say where you should move it too.
>>
> I renamed this function to IndexRelationGetParentRelation and muved to
> relcache.c
>

Thanks, it looks less out of place there than it did in nbtinsert.c

> I don't call a quote_identifier on only data error properties like
> table_name or schema_name (but I am open to arguments for it or
> against it). The quote_identifier is used for column names, because
> there should be a more names and comma should be used inside name -
> and this is consistent with pg_get_indexdef_columns.
>
> Regards
>

Okay.

> Pavel Stehule
>
>

I'm going to mark this as ready for a committer.

2011/6/21 Steve Singer <ssinger_pg(at)sympatico(dot)ca>:
> On 11-06-20 03:44 PM, Pavel Stehule wrote:
>
> Hello
>
> You need to update config.sgml at the same time you update this format.
> You need to append a "," after application name but before constraintName.
> As it stands the CSV log has something like:
> .....nbtinsert.c:433","psql""a_pkey","public","a","a"
>
> fixed
>
>
> The CSV log seems fine now.
>
>
>
> nbtinsert.c
>
> pg_get_indrelation is named differently than everything else in this file
> (ie _bt...).  My guess is that this function belongs somewhere else but I
> don't know the code well enough to say where you should move it too.
>
> I renamed this function to IndexRelationGetParentRelation and muved to
> relcache.c
>
>
> Thanks, it looks less out of place there than it did in nbtinsert.c
>
> I don't call a quote_identifier on only data error properties like
> table_name or schema_name (but I am open to arguments for it or
> against it). The quote_identifier is used for column names, because
> there should be a more names and comma should be used inside name -
> and this is consistent with pg_get_indexdef_columns.
>
> Regards
>
>
> Okay.
>
>
>
> Pavel Stehule
>
>
> I'm going to mark this as ready for a committer.
>

Thank you very much

Regards

Pavel Stehule

>
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> I am sending a updated patch

I looked over this patch a bit. I guess my main concern about it
is that the set of items to be reported seems to have been made up on
a whim. I think that we ought to follow the SQL standard, which has a
pretty clearly defined set of additional information items --- look at
the spec for the GET DIAGNOSTICS statement. (In SQL:2008, this is
section 23.1 <get diagnostics statement>.) I don't feel that we need to
implement every field the standard calls for, at least not right away,
but we ought to have their list in mind. Conversely, implementing items
that *aren't* listed in the spec has to meet a considerably higher bar
than somebody just submitting a patch that does it.

The standard information items that seem reasonable for us to implement
in the near future are

COLUMN_NAME
CONSTRAINT_NAME
CONSTRAINT_SCHEMA
SCHEMA_NAME
TABLE_NAME
TRIGGER_NAME
TRIGGER_SCHEMA

So I'd like to see the patch revised to use this terminology. We
probably also need to think a bit harder about the PG_DIAG_XXX code
letters to be used --- we're already just about at the limit of what
fields can have reasonably-mnemonic code letters, and not all of the
above have obvious choices, let alone the rest of what's in the spec
that we might someday want to implement. What assignment rule should
we use when we can't choose a mnemonic letter?

Some other specific comments on the patch follow:

1. It's way short in the documentation department. protocol.sgml
certainly needs additions (see "Error and Notice Message Fields"),
also libpq.sgml's discussion of PQresultErrorField(), also
sources.sgml's "Reporting Errors Within the Server", and I'm not
sure where else.

2. I think you could drop the tuple-descriptor changes, because they're
only needed in service of an information item that is not found in the
standard and doesn't seem very necessary. The standard says to report
the name of the constraint, not what columns it involves.

3. errrel() is extremely poorly considered. The fact that it requires
utils/relcache.h to be #included by elog.h (and therefore by *every*
*single* *file* in the backend) is a symptom of that, but expecting
elog.c to do catalog lookups is as bad or worse from a modularity
standpoint. I think all the added elog functions should not take
anything higher-level than a C string.

4. Actually, it would probably be a good idea to avoid inventing a new
elog API function for each individual new information item; something
along the lines of "erritem(PG_DIAG_WHATEVER, string_value)" would be
more appropriate to cover the inevitable future expansions.

5. I don't think IndexRelationGetParentRelation is very appropriate
either --- in the use cases you have, the parent table's OID is easily
accessible, as is its namespace (which'll be the same as the index's)
and so you could just have the callers do get_rel_name(tableoid).
Doing a relcache open in an error reporting path seems like overkill.

I'm going to mark this patch Returned With Feedback.

regards, tom lane

Hello Tom,

Thank you for review

I am thinking, so your comment is clean and I'll respect it in new version.

There is only one issue, that should be solved first. I introduced non
standard diagnostics field "column_names", because there is not
possible get "column_name" value for check constraints now. A correct
implementation of COLUMN_NAME field needs a explicit relation between
pg_constraint and pg_attribute - maybe implemented as new column to
pg_constraint. Do you agree?

Regards

Pavel

2011/7/16 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> I am sending a updated patch
>
> I looked over this patch a bit.  I guess my main concern about it
> is that the set of items to be reported seems to have been made up on
> a whim.  I think that we ought to follow the SQL standard, which has a
> pretty clearly defined set of additional information items --- look at
> the spec for the GET DIAGNOSTICS statement.  (In SQL:2008, this is
> section 23.1 <get diagnostics statement>.)  I don't feel that we need to
> implement every field the standard calls for, at least not right away,
> but we ought to have their list in mind.  Conversely, implementing items
> that *aren't* listed in the spec has to meet a considerably higher bar
> than somebody just submitting a patch that does it.
>
> The standard information items that seem reasonable for us to implement
> in the near future are
>
>        COLUMN_NAME
>        CONSTRAINT_NAME
>        CONSTRAINT_SCHEMA
>        SCHEMA_NAME
>        TABLE_NAME
>        TRIGGER_NAME
>        TRIGGER_SCHEMA
>
> So I'd like to see the patch revised to use this terminology.  We
> probably also need to think a bit harder about the PG_DIAG_XXX code
> letters to be used --- we're already just about at the limit of what
> fields can have reasonably-mnemonic code letters, and not all of the
> above have obvious choices, let alone the rest of what's in the spec
> that we might someday want to implement.  What assignment rule should
> we use when we can't choose a mnemonic letter?
>

> Some other specific comments on the patch follow:
>
> 1. It's way short in the documentation department.  protocol.sgml
> certainly needs additions (see "Error and Notice Message Fields"),
> also libpq.sgml's discussion of PQresultErrorField(), also
> sources.sgml's "Reporting Errors Within the Server", and I'm not
> sure where else.
>

ok

> 2. I think you could drop the tuple-descriptor changes, because they're
> only needed in service of an information item that is not found in the
> standard and doesn't seem very necessary.  The standard says to report
> the name of the constraint, not what columns it involves.
>
> 3. errrel() is extremely poorly considered.  The fact that it requires
> utils/relcache.h to be #included by elog.h (and therefore by *every*
> *single* *file* in the backend) is a symptom of that, but expecting
> elog.c to do catalog lookups is as bad or worse from a modularity
> standpoint.  I think all the added elog functions should not take
> anything higher-level than a C string.
>
> 4. Actually, it would probably be a good idea to avoid inventing a new
> elog API function for each individual new information item; something
> along the lines of "erritem(PG_DIAG_WHATEVER, string_value)" would be
> more appropriate to cover the inevitable future expansions.
>
> 5. I don't think IndexRelationGetParentRelation is very appropriate
> either --- in the use cases you have, the parent table's OID is easily
> accessible, as is its namespace (which'll be the same as the index's)
> and so you could just have the callers do get_rel_name(tableoid).
> Doing a relcache open in an error reporting path seems like overkill.
>
> I'm going to mark this patch Returned With Feedback.
>
>                        regards, tom lane
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> There is only one issue, that should be solved first. I introduced non
> standard diagnostics field "column_names", because there is not
> possible get "column_name" value for check constraints now. A correct
> implementation of COLUMN_NAME field needs a explicit relation between
> pg_constraint and pg_attribute - maybe implemented as new column to
> pg_constraint. Do you agree?

No, I don't. You're adding complication to solve a problem that doesn't
need to be solved. The standard says to return the name of the
constraint for a constraint-violation failure. It does not say anything
about naming the associated column(s). COLUMN_NAME is only supposed to
be defined for certain kinds of errors, and this isn't one of them.

regards, tom lane

Tom,

> No, I don't. You're adding complication to solve a problem that doesn't
> need to be solved. The standard says to return the name of the
> constraint for a constraint-violation failure. It does not say anything
> about naming the associated column(s). COLUMN_NAME is only supposed to
> be defined for certain kinds of errors, and this isn't one of them.

Are we talking about FK constraints here, or CHECK contstraints?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> writes:
> Tom,
>> No, I don't. You're adding complication to solve a problem that doesn't
>> need to be solved. The standard says to return the name of the
>> constraint for a constraint-violation failure. It does not say anything
>> about naming the associated column(s). COLUMN_NAME is only supposed to
>> be defined for certain kinds of errors, and this isn't one of them.

> Are we talking about FK constraints here, or CHECK contstraints?

Either one. They both have the potential to reference more than one
column, so if the committee had meant errors to try to identify the
referenced columns, they'd have put something other than COLUMN_NAME
into the standard. They didn't.

regards, tom lane

2011/7/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Josh Berkus <josh(at)agliodbs(dot)com> writes:
>> Tom,
>>> No, I don't.  You're adding complication to solve a problem that doesn't
>>> need to be solved.  The standard says to return the name of the
>>> constraint for a constraint-violation failure.  It does not say anything
>>> about naming the associated column(s).  COLUMN_NAME is only supposed to
>>> be defined for certain kinds of errors, and this isn't one of them.
>
>> Are we talking about FK constraints here, or CHECK contstraints?
>
> Either one.  They both have the potential to reference more than one
> column, so if the committee had meant errors to try to identify the
> referenced columns, they'd have put something other than COLUMN_NAME
> into the standard.  They didn't.

Personally, I see a sense for COLUMN_NAME field only with relation to
CHECK_CONSTRAINT - for any other constraint using a COLUMN_NAME is
based on parsing a constraint rule - and I don't believe so the
standard is based in it. Column check constraint is attached
explicitly to one column - but this relation should not be based on
semantic.

We can check DB2 implementation.

Regards
Pavel

>
>                        regards, tom lane
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2011/7/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> Are we talking about FK constraints here, or CHECK contstraints?

>> Either one. They both have the potential to reference more than one
>> column, so if the committee had meant errors to try to identify the
>> referenced columns, they'd have put something other than COLUMN_NAME
>> into the standard. They didn't.

> Personally, I see a sense for COLUMN_NAME field only with relation to
> CHECK_CONSTRAINT - for any other constraint using a COLUMN_NAME is
> based on parsing a constraint rule - and I don't believe so the
> standard is based in it.

Read the standard. COLUMN_NAME is defined for use only in
syntax_error_or_access_rule_violation errors that relate to a specific
column. In fact, the spec is written as (SQL:2008 23.1 GR 4-h-ii):

If the syntax error or access rule violation was for an inaccessible
column, then the value of COLUMN_NAME is the <column name> of that
column. Otherwise, the value of COLUMN_NAME is a zero-length string.

which suggests that it might be meant *only* for use with
INSUFFICIENT_PRIVILEGE errors that are thrown due to a column ACL.
We can probably extend that to some other syntax errors, like unknown
column or wrong datatype or what have you, but there is nothing here to
suggest that we have to force the issue for errors that don't naturally
relate to exactly one column. And CHECK constraints don't. Consider
"CHECK (f1 > f2)".

regards, tom lane

2011/7/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2011/7/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>>> Are we talking about FK constraints here, or CHECK contstraints?
>
>>> Either one.  They both have the potential to reference more than one
>>> column, so if the committee had meant errors to try to identify the
>>> referenced columns, they'd have put something other than COLUMN_NAME
>>> into the standard.  They didn't.
>
>> Personally, I see a sense for COLUMN_NAME field only with relation to
>> CHECK_CONSTRAINT - for any other constraint using a COLUMN_NAME is
>> based on parsing a constraint rule - and I don't believe so the
>> standard is based in it.
>
> Read the standard.  COLUMN_NAME is defined for use only in
> syntax_error_or_access_rule_violation errors that relate to a specific
> column.  In fact, the spec is written as (SQL:2008 23.1 GR 4-h-ii):
>
>        If the syntax error or access rule violation was for an inaccessible
>        column, then the value of COLUMN_NAME is the <column name> of that
>        column. Otherwise, the value of COLUMN_NAME is a zero-length string.
>
> which suggests that it might be meant *only* for use with
> INSUFFICIENT_PRIVILEGE errors that are thrown due to a column ACL.
> We can probably extend that to some other syntax errors, like unknown
> column or wrong datatype or what have you, but there is nothing here to
> suggest that we have to force the issue for errors that don't naturally
> relate to exactly one column.  And CHECK constraints don't.  Consider
> "CHECK (f1 > f2)".
>

ok, this is relative clean, but

so for example, NULL or DOMAIN constraints doesn't affect a
COLUMN_NAME? These constraints has no name.

regards

Pavel

>                        regards, tom lane
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> so for example, NULL or DOMAIN constraints doesn't affect a
> COLUMN_NAME? These constraints has no name.

Well, the executor's NOT NULL tests could certainly be extended to emit
COLUMN_NAME --- I don't see any logical or implementation problem with
that, even if it seems to be outside the scope of what the standard says
to use the field for. But let's not get into modifying the system
catalogs to produce error fields that are not required by the standard.

regards, tom lane

Tom,

> Either one. They both have the potential to reference more than one
> column, so if the committee had meant errors to try to identify the
> referenced columns, they'd have put something other than COLUMN_NAME
> into the standard. They didn't.

I'm less concerned about the standard here and more concerned about what
helps our users. Having column names for an FK error is *extremely*
useful for troubleshooting, particularly if the database has been
upgraded from the 7.4 days and has non-useful FK names like "$3".

I agree that column names for CHECK constraints is a bit of a tar baby,
since check constraints can be on complex permutations of columns.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Josh Berkus <josh(at)agliodbs(dot)com> wrote:

> I'm less concerned about the standard here and more concerned
> about what helps our users. Having column names for an FK error
> is *extremely* useful for troubleshooting, particularly if the
> database has been upgraded from the 7.4 days and has non-useful FK
> names like "$3".

If it gives a FK constraint name, isn't there a way to get from that
to the columns used by the constraint? If we want to support
something non-standard, we can always tell them to look at the text
of the error detail, right?

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> writes:
> Josh Berkus <josh(at)agliodbs(dot)com> wrote:
>> I'm less concerned about the standard here and more concerned
>> about what helps our users. Having column names for an FK error
>> is *extremely* useful for troubleshooting, particularly if the
>> database has been upgraded from the 7.4 days and has non-useful FK
>> names like "$3".

> If it gives a FK constraint name, isn't there a way to get from that
> to the columns used by the constraint? If we want to support
> something non-standard, we can always tell them to look at the text
> of the error detail, right?

Yes. This is entirely *not* about friendliness to human users; they're
going to read the existing primary/detail/hint fields, and probably
aren't even going to see these new error fields by default. What the
new fields are meant for is allowing client programs to do something
useful without parsing the text of the human-oriented fields ... for
instance, identify which FK constraint got violated. Somebody who's
intending to use this functionality would presumably take care to give
his constraints names that were helpful for his purposes. Moreover,
if he's hoping to use that client code against more than one database,
what he's going to want is SQL-standard functionality, not more nor less.

As for the "my constraints have names like $3" argument, maybe an ALTER
CONSTRAINT RENAME command would be the most helpful answer.

regards, tom lane

Excerpts from Pavel Stehule's message of lun jul 18 16:02:43 -0400 2011:
> 2011/7/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> > which suggests that it might be meant *only* for use with
> > INSUFFICIENT_PRIVILEGE errors that are thrown due to a column ACL.
> > We can probably extend that to some other syntax errors, like unknown
> > column or wrong datatype or what have you, but there is nothing here to
> > suggest that we have to force the issue for errors that don't naturally
> > relate to exactly one column.  And CHECK constraints don't.  Consider
> > "CHECK (f1 > f2)".
>
> ok, this is relative clean, but
>
> so for example, NULL or DOMAIN constraints doesn't affect a
> COLUMN_NAME? These constraints has no name.

I dunno about domains, but NOT NULL constraints definitely have names
according to the standard (and will have them in PG soon enough).

Hmm, domain constraints are CHECK or NOT NULL, and both of them have or
will have names.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Hello

this is a refreshed patch. Only constraints and RI is supported now.
There is about 1000 ereport calls, where a enhanced diagnostics should
be used, but probably we don't modify all in one time.

Regards

Pavel Stehule

2011/7/19 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of lun jul 18 16:02:43 -0400 2011:
>> 2011/7/18 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
>> > which suggests that it might be meant *only* for use with
>> > INSUFFICIENT_PRIVILEGE errors that are thrown due to a column ACL.
>> > We can probably extend that to some other syntax errors, like unknown
>> > column or wrong datatype or what have you, but there is nothing here to
>> > suggest that we have to force the issue for errors that don't naturally
>> > relate to exactly one column.  And CHECK constraints don't.  Consider
>> > "CHECK (f1 > f2)".
>>
>> ok, this is relative clean, but
>>
>> so for example, NULL or DOMAIN constraints doesn't affect a
>> COLUMN_NAME? These constraints has no name.
>
> I dunno about domains, but NOT NULL constraints definitely have names
> according to the standard (and will have them in PG soon enough).
>
> Hmm, domain constraints are CHECK or NOT NULL, and both of them have or
> will have names.
>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

On Jul27, 2011, at 23:20 , Pavel Stehule wrote:
> this is a refreshed patch. Only constraints and RI is supported now.
> There is about 1000 ereport calls, where a enhanced diagnostics should
> be used, but probably we don't modify all in one time.

I wonder if it wouldn't be better to have something like the machinery
around ErrorContextCallback to fill in the constraint details. You'd then
only need to modify the places which initiate constraint checks, instead
of every single ereport() in the constraint implementations.

Just a wild idea, though - I haven't check whether this is actually
feasible or no.

best regards,
Florian Pflug

2011/7/28 Florian Pflug <fgp(at)phlo(dot)org>:
> On Jul27, 2011, at 23:20 , Pavel Stehule wrote:
>> this is a refreshed patch. Only constraints and RI is supported now.
>> There is about 1000 ereport calls, where a enhanced diagnostics should
>> be used, but probably we don't modify all in one time.
>
> I wonder if it wouldn't be better to have something like the machinery
> around ErrorContextCallback to fill in the constraint details. You'd then
> only need to modify the places which initiate constraint checks, instead
> of every single ereport() in the constraint implementations.
>
> Just a wild idea, though - I haven't check whether this is actually
> feasible or no.

I though about this too, but sometimes is relative difficult to
specify a fields before exception -- see a ri_triggers part.
TABLE_NAME and TABLE_SCHEMA should not contains a name of processed
table, but name of error, that is related to error. It can be
different. But if we would to use a enhanced errors for "in"
functions, then some injection into ErrorContextCallback should be
necessary - but again - the these fields are no related to function's
scope - so it mean a more manipulation with ErrorContext.

Regards

Pavel Stehule

>
> best regards,
> Florian Pflug
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>