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

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

ok

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