Re: patch for 9.2: enhanced errors

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

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

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

COLUMN_NAME
CONSTRAINT_NAME
CONSTRAINT_SCHEMA
SCHEMA_NAME
TABLE_NAME
TRIGGER_NAME
TRIGGER_SCHEMA

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

Some other specific comments on the patch follow:

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

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

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

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

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

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

regards, tom lane