Re: enhanced error fields - Mailing list pgsql-hackers

On 10 October 2012 14:56, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> (eelog3.diff)

This looks better.

You need a better comment here:

+  * relerror.c
+  *      relation error loging functions
+  *

I'm still not satisfied with the lack of firm guarantees about what
errcodes one can assume these fields will be available for. I suggest
that this be explicitly documented within errcodes.h. For example,
right now if some client code wants to discriminate against a certain
check constraint in its error-handling code (typically to provide a
user-level message), it might do something like this:

try
{...
}
catch(CheckViolation e)
{    // This works:    if (e.constraint_name == "postive_balance")        MessageBox("Your account must have a positive
balance.");   // This is based on a check constraint in a domain, and is
 
therefore broken right now:    else if (e.constraint_name == "valid_barcode")        MessageBox("You inputted an
invalidbarcode - check digit was wrong");
 
}

This is broken right now, simply because the application cannot rely
on the constraint name being available, since for no particular reason
some of the ERRCODE_CHECK_VIOLATION ereport sites (like in execQual.c)
don't provide an errconstraint(). What is needed is a coding standard
that says "ERRCODE_CHECK_VIOLATION ereport call sites need to have an
errconstraint()". Without this, the patch is quite pointless.

My mind is not 100% closed to the idea that we provide these extra
fields on a "best-effort" basis per errcode, but it's pretty close to
there. Why should we allow this unreasonable inconsistency? The usage
pattern that I describe above is a real one, and I thought that the
whole point was to support it.

I have previously outlined places where this type of inconsistency
exists, in an earlier revision. [1]

If you want to phase in the introduction of requiring that all
relevant callsites use this infrastructure, I guess I'm okay with
that. However, I'm going to have to insist that for each of these new
fields, for any errcode you identify as requiring the field, either
all callsites get all relevant fields, or no call sites get all
relevant fields, and that each errcode be documented as such. So you
should probably just bite the bullet and figure out a reasonable and
comprehensive set of rules on adding these fields based on errcode.
Loosey goosey isn't going to cut it.

I'm having a difficult time imagining why we'd only have the
constraint/tablename for these errcodes (with one exception, noted
below):

/* Class 23 - Integrity Constraint Violation */
#define ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION
MAKE_SQLSTATE('2','3','0','0','0')
#define ERRCODE_RESTRICT_VIOLATION MAKE_SQLSTATE('2','3','0','0','1')
#define ERRCODE_NOT_NULL_VIOLATION MAKE_SQLSTATE('2','3','5','0','2')
#define ERRCODE_FOREIGN_KEY_VIOLATION MAKE_SQLSTATE('2','3','5','0','3')
#define ERRCODE_UNIQUE_VIOLATION MAKE_SQLSTATE('2','3','5','0','5')
#define ERRCODE_CHECK_VIOLATION MAKE_SQLSTATE('2','3','5','1','4')
#define ERRCODE_EXCLUSION_VIOLATION MAKE_SQLSTATE('2','3','P','0','1')

You previously defending some omissions [2] on the basis that they
involved domains, so some fields were unavailable. This doesn't appear
to be quite valid, though. For example, consider this untouched
callsite within execQual, that relates to a domain:
                if (!conIsNull &&                    !DatumGetBool(conResult))                    ereport(ERROR,
                   (errcode(ERRCODE_CHECK_VIOLATION),                             errmsg("value for domain %s violates
checkconstraint \"%s\"",                                    format_type_be(ctest->resulttype),
         con->name)));
 

There is no reason why you couldn't have at least given the constraint
name. It might represent an unreasonable burden for you to determine
the table that these constraints relate to by going through the rabbit
hole of executor state, since we haven't had any complaints about this
information being available within error messages before, AFAIK. If
that is the case, the general non-availability of this information for
domains needs to be documented. I guess that's logical enough, since
there doesn't necessarily have to be a table involved in the event of
a domain constraint violation. However, there does have to be a
constraint, for example, involved.

FWIW, I happen to think that not-null constraints at the domain level
are kind of stupid (though check constraints are great), but what do I
know...

Anyway, the bottom line is that authors of Postgres client libraries
(and their users) ought to have a reasonable set of guarantees about
when this information is available. If that means you have to make one
or two explicit, documented exceptions to my previous "all or nothing"
proviso, such as "table names won't be available in the event of
domain constraints", so be it.

I'm going to suggest you add a note to both the docs and errcodes.h
regarding all this in your next revision. People need to be adding
these fields for all errcodes that *require* them going forward. If,
in the future, ERRCODE_UNIQUE_VIOLATION errors, for example, cannot
supply a constraint name that was violated, then that is, almost by
definition, the wrong errcode to use.

[1] http://archives.postgresql.org/message-id/CAEYLb_VJK+AZe6fO_s0Md0ge5D=RenDf7wg+g4NxN8mhKQ4Gzg@mail.gmail.com

[2] CAFj8pRDtTDvoSvJT8PP08mQ_LW2HaOmWXvRUdoYLhk9xF7KMyw@mail.gmail.com

-- 
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services