Re: [HACKERS] patch for 9.2: enhanced errors
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
2011/7/28 Florian Pflug f...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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 t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
2011/7/18 Tom Lane t...@sss.pgh.pa.us: Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Pavel Stehule pavel.steh...@gmail.com writes: 2011/7/18 Tom Lane t...@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). 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
Re: [HACKERS] patch for 9.2: enhanced errors
2011/7/18 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: 2011/7/18 Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Pavel Stehule pavel.steh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Kevin Grittner kevin.gritt...@wicourts.gov writes: Josh Berkus j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Excerpts from Pavel Stehule's message of lun jul 18 16:02:43 -0400 2011: 2011/7/18 Tom Lane t...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Pavel Stehule pavel.steh...@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. 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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,psqla_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.
Re: [HACKERS] patch for 9.2: enhanced errors
2011/6/21 Steve Singer ssinger...@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,psqla_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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,psqla_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
*** ./doc/src/sgml/config.sgml.orig 2011-06-20 18:08:39.0 +0200
--- ./doc/src/sgml/config.sgml 2011-06-20 21:12:31.688165497 +0200
***
*** 3919,3927
user query that led to the error (if any and enabled by
varnamelog_min_error_statement/),
character count of the error position therein,
! location of the error in the PostgreSQL source code
(if varnamelog_error_verbosity/ is set to literalverbose/),
! and application name.
Here is a sample table definition for storing CSV-format log output:
programlisting
--- 3919,3933
user query that led to the error (if any and enabled by
varnamelog_min_error_statement/),
character count of the error position therein,
! location of the error in the PostgreSQL source code,
(if varnamelog_error_verbosity/ is set to literalverbose/),
! application name,
! (following fields to end are filled when varnamelog_error_verbosity/ is
! set to literalverbose/)
! constraint name,
! schema name,
! table name,
! and column names.
Here is a sample table definition for storing CSV-format log output:
programlisting
***
*** 3950,3955
--- 3956,3965
query_pos integer,
location text,
application_name text,
+ constraint_name text,
+ schema_name text,
+ table_name text,
+ column_names text,
PRIMARY KEY (session_id, session_line_num)
);
/programlisting
*** ./src/backend/access/common/tupdesc.c.orig 2011-06-20 18:08:39.0 +0200
--- ./src/backend/access/common/tupdesc.c 2011-06-20 20:30:15.477883102 +0200
***
*** 200,205
--- 200,217
cpy-check[i].ccname = pstrdup(constr-check[i].ccname);
if (constr-check[i].ccbin)
cpy-check[i].ccbin = pstrdup(constr-check[i].ccbin);
+ if (constr-check[i].nkeys 0)
+ {
+ cpy-check[i].conkey = palloc(sizeof(int16) * constr-check[i].nkeys);
+ memcpy(cpy-check[i].conkey, constr-check[i].conkey,
+ sizeof(int16) * constr-check[i].nkeys);
+ cpy-check[i].nkeys = constr-check[i].nkeys;
+ }
+ else
+ {
+ cpy-check[i].conkey = NULL;
+ constr-check[i].nkeys = 0;
+ }
}
}
***
*** 249,254
--- 261,268
pfree(check[i].ccname);
if (check[i].ccbin)
pfree(check[i].ccbin);
+ if (check[i].conkey)
+ pfree(check[i].conkey);
}
pfree(check);
}
***
*** 409,414
--- 423,431
* Similarly, don't assume that the checks are always read in the
* same order; match them up by name and contents. (The name
* *should* be unique, but...)
+ *
+ * nkeys and conkey depends on ccbin, and there are not neccessary
+ * to compare it.
*/
for (j = 0; j n; check2++, j++)
{
*** ./src/backend/access/nbtree/nbtinsert.c.orig 2011-06-20 18:08:39.0 +0200
---
Re: [HACKERS] patch for 9.2: enhanced errors
Hello 2011/6/19 Steve Singer ssinger...@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,psqla_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
2011/6/19 Steve Singer ssinger...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
2011/6/19 Florian Pflug f...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
On Jun19, 2011, at 14:03 , Pavel Stehule wrote: 2011/6/19 Florian Pflug f...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
2011/6/19 Florian Pflug f...@phlo.org: On Jun19, 2011, at 14:03 , Pavel Stehule wrote: 2011/6/19 Florian Pflug f...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
Excerpts from Pavel Stehule's message of dom jun 19 06:51:13 -0400 2011: Hello 2011/6/19 Steve Singer ssinger...@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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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,psqla_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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] patch for 9.2: enhanced errors
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] 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: 0: 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
*** ./src/backend/access/nbtree/nbtinsert.c.orig 2011-04-27 23:17:22.0 +0200
--- ./src/backend/access/nbtree/nbtinsert.c 2011-06-08 21:57:45.616691664 +0200
***
*** 23,28
--- 23,30
#include storage/lmgr.h
#include storage/predicate.h
#include utils/inval.h
+ #include utils/builtins.h
+ #include utils/syscache.h
#include utils/tqual.h
***
*** 83,88
--- 85,121
/*
+ * Returns a parent relation of index
+ */
+ Relation
+ pg_get_indrelation(Relation idxrel)
+ {
+ HeapTuple ht_idx;
+ Form_pg_index idxrec;
+ Oid indrelid;
+ Relation r;
+
+ /*
+ * Fetch the pg_index tuple by the Oid of the index
+ */
+ ht_idx = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(RelationGetRelid(idxrel)));
+ if (!HeapTupleIsValid(ht_idx))
+ elog(ERROR, cache lookup failed for index %u, RelationGetRelid(idxrel));
+ idxrec = (Form_pg_index) GETSTRUCT(ht_idx);
+
+ indrelid = idxrec-indrelid;
+ r = RelationIdGetRelation(indrelid);
+
+ if (!RelationIsValid(r))
+ elog(ERROR, could not open relation with OID %u, indrelid);
+
+ /* Clean up */
+ ReleaseSysCache(ht_idx);
+
+ return r;
+ }
+
+ /*
* _bt_doinsert() -- Handle insertion of a single index tuple in the tree.
*
* This routine is called by the public interface routines, btbuild
***
*** 394,400
RelationGetRelationName(rel)),
errdetail(Key %s already exists.,
BuildIndexValueDescription(rel,
! values, isnull;
}
}
else if (all_dead)
--- 427,436
RelationGetRelationName(rel)),
errdetail(Key %s already exists.,
BuildIndexValueDescription(rel,
! values, isnull)),
! errconstrname(RelationGetRelationName(rel)),
! errrel(pg_get_indrelation(rel)),
! errcolnames(pg_get_indexdef_columns(RelationGetRelid(rel), true;
}
}
else if (all_dead)
***
*** 534,540
RelationGetRelationName(rel)),
errhint(Values larger than 1/3 of a buffer page cannot be indexed.\n
Consider a function index of an MD5 hash of the value,
! or use full text indexing.)));
/*--
* If we will need to split the page to put the item on this page,
--- 570,578
RelationGetRelationName(rel)),
errhint(Values larger than 1/3 of a buffer page cannot be indexed.\n
Consider a function index of an MD5 hash of the value,
! or use full text indexing.),
! errconstrname(RelationGetRelationName(rel)),
! errrel(pg_get_indrelation(rel;
/*--
* If we will need to split the page to put the item on this page,
*** ./src/backend/executor/execMain.c.orig 2011-04-27 23:17:22.0 +0200
--- ./src/backend/executor/execMain.c 2011-06-08 18:51:19.492670762 +0200
***
*** 1433,1439
/*
* ExecRelCheck --- check that tuple meets constraints for result relation
*/
! static const char *
ExecRelCheck(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
--- 1433,1439
/*
* ExecRelCheck --- check that tuple meets constraints for result relation
*/
! static ConstrCheck *
ExecRelCheck(ResultRelInfo *resultRelInfo,
TupleTableSlot *slot, EState *estate)
{
***
*** 1485,1491
* ExecQual to return TRUE for NULL.
*/
if (!ExecQual(qual, econtext, true))
! return check[i].ccname;
}
/* NULL result means no error */
--- 1485,1491
* ExecQual to
