Thread: 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
Attachment
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@sympatico.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@sympatico.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 numbersfrom the c file are also included? If so then this won't work for the regression tests. Having the diff comparisonfail 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@phlo.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 numbersfrom the c file are also included? If so then this won't work for the regression tests. Having the diff comparisonfail 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@phlo.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@phlo.org>:
> On Jun19, 2011, at 14:03 , Pavel Stehule wrote:
>> 2011/6/19 Florian Pflug <fgp@phlo.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@sympatico.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@commandprompt.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
Attachment
On 11-06-20 03:44 PM, Pavel Stehule wrote: <blockquote cite="mid:BANLkTik0S1kzyLFG1Pv6KhTNMqqUHh-zkA@mail.gmail.com" type="cite"><prewrap="">Hello </pre><blockquote type="cite"><pre wrap=""> 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" </pre></blockquote><pre wrap=""> fixed </pre></blockquote><br /> The CSV log seems fine now.<br /><br /><br /><blockquote cite="mid:BANLkTik0S1kzyLFG1Pv6KhTNMqqUHh-zkA@mail.gmail.com"type="cite"><pre wrap=""></pre><blockquote type="cite"><prewrap=""> 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. </pre></blockquote><pre wrap=""> I renamed this function to IndexRelationGetParentRelation and muved to relcache.c </pre></blockquote><br /> Thanks, it looks less out of place there than it did in nbtinsert.c<br /><br /><blockquote cite="mid:BANLkTik0S1kzyLFG1Pv6KhTNMqqUHh-zkA@mail.gmail.com"type="cite"><pre wrap=""> 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 </pre></blockquote><br /> Okay. <br /><br /><br /><br /><blockquote cite="mid:BANLkTik0S1kzyLFG1Pv6KhTNMqqUHh-zkA@mail.gmail.com"type="cite"><pre wrap="">Pavel Stehule </pre> <pre wrap=""> <fieldset class="mimeAttachmentHeader"></fieldset> </pre></blockquote><br /> I'm going to mark this as ready for a committer.<br /><br /><br />
2011/6/21 Steve Singer <ssinger_pg@sympatico.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.stehule@gmail.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_NAMECONSTRAINT_NAMECONSTRAINT_SCHEMASCHEMA_NAMETABLE_NAMETRIGGER_NAMETRIGGER_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@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.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.stehule@gmail.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@agliodbs.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@sss.pgh.pa.us>: > Josh Berkus <josh@agliodbs.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2011/7/18 Tom Lane <tgl@sss.pgh.pa.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 inaccessiblecolumn, then the value of COLUMN_NAME is the
<columnname> of thatcolumn. 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@sss.pgh.pa.us>: > Pavel Stehule <pavel.stehule@gmail.com> writes: >> 2011/7/18 Tom Lane <tgl@sss.pgh.pa.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.stehule@gmail.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@agliodbs.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.Grittner@wicourts.gov> writes:
> Josh Berkus <josh@agliodbs.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@sss.pgh.pa.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@commandprompt.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@commandprompt.com>: > Excerpts from Pavel Stehule's message of lun jul 18 16:02:43 -0400 2011: >> 2011/7/18 Tom Lane <tgl@sss.pgh.pa.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@commandprompt.com> > The PostgreSQL Company - Command Prompt, Inc. > PostgreSQL Replication, Consulting, Custom Development, 24x7 support >
Attachment
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@phlo.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >