Re: enhanced error fields

Hello

2012/10/11 Peter Geoghegan <peter(at)2ndquadrant(dot)com>:
> On 10 October 2012 14:56, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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 invalid barcode - 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.

I understand to your request, but I don't thing so this request is
100% valid. Check violation is good example. Constraint names are
"optional" in PostgreSQL - so we cannot require constraint_name. One
from first prototypes I used generated name for NULL constraints and
it was rejected - because It can be confusing, because a user doesn't
find these names in catalogue. I agree with it now - it better show
nothing, than show some phantom. More - a design of these feature from
SQL/PSM and ANSI/SQL is not too strict. There is no exception, when
you asking any unfilled value - you get a empty string instead. And
more - there are no info in standard, what fields are optional and
what fields are mandatory.

And although we don't checking consistence of exception fields, I
think so this patch is very usable. I have a three text fields now:
message, detail, hint - and I can do same error, that you are
described. This patch doesn't change it. But it creates a few new
basic variables (for all possible exceptions), that can be used for
simplification of error processing. It is not silver bullet. And it is
not C++. Creating some new tool for checking consistency of exceptions
is not good way - and you are newer ensure consistency of custom
exceptions.

>
> 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')

ERRCODE_UNIQUE_VIOLATION and ERRCODE_EXCLUSION_VIOLATION should be
related to index relation, not parent relation. Then we don't need set
COLUMN_NAME, that can be expression or more columns.

>
> 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 check constraint \"%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.

yes, CONSTRAINT_NAME in this case should be used. TABLE_NAME can be or
should not be empty, but this information is not available, because
some facts can be changed in rewriter stage.

>
> 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.

I can agree, so some documentation is necessary (maybe some table) -
now we have not described context of all errors. Other needs a
searching of some consensus - or searching solution - our syntax
allows some variations that are unsupported in ANSI/SQL - and then we
have to use some generated name or we don't show this information. It
is a basic and most important question. So first we have to find reply
to following question: this patch should to follow our current
implementation of exceptions or we modify exceptions to be more close
to ANSI/SQL (and we have to modify model)?

Regards

Pavel

>
> [1] http://archives.postgresql.org/message-id/CAEYLb_VJK+AZe6fO_s0Md0ge5D=RenDf7wg+g4NxN8mhKQ4Gzg@mail.gmail.com
>
> [2] CAFj8pRDtTDvoSvJT8PP08mQ_LW2HaOmWXvRUdoYLhk9xF7KMyw(at)mail(dot)gmail(dot)com
>
> --
> Peter Geoghegan http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training and Services