Re: enhanced error fields

On 12 October 2012 20:27, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> 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.

That's beside the point. NOT NULL constraints are not catalogued (for
now), so sure, the only reasonable thing to do is to have an empty
string in that case. Since no one expects to be able to get the name
of a NOT NULL constraint anyway, that isn't a problem.

Once again, the problem, in particular, is that there is no
well-defined set of rules that client code can follow to be sure that
a field name they're interested in will be available at all. In all
revisions thus far, you have seemingly arbitrarily decided to not add
some some fields in some places. I mentioned already that some
ERRCODE_CHECK_VIOLATION sites didn't name a constraint - other places
don't name a table when one is available, as with some
ERRCODE_NOT_NULL_VIOLATION sites. These fields need to be added, and
what's more, the rules for where they need to be added need to be
coherently described. So, that's about 3 sentences of extra
documentation, saying to both users and hackers (at the very least):

* NOT NULL constraints won't have a CONSTRAINT_NAME available, since
they aren't catalogued.

* Domains won't have a TABLE_NAME available, even though there may
actually be a table name associated with the error.

Have I missed one?

That all seems pretty simple to me, and I don't see what the problem is.

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

The simplification of error processing is that they can now reliably
get these fields - they don't have to use some kludge like parsing a
(possibly localised) error message to look for a check constraint
name. I'm not asking you to add run-time verification - I'm asking you
to institute a coding standard, that is limited to backend code, and
to document what assumptions applications can make.

To my mind, if the user cannot rely on the fields accurately
indicating error conditions according to some coherent set of rules
(in actuality, one or two simple and obvious exceptions, only one of
which is slightly surprising), then this patch is not only not
helpful, it's harmful. If they're only available according to some
completely arbitrary and obscure criteria, (like the fact that you've
included one ERRCODE_CHECK_VIOLATION site but not another), that's a
footgun.

> Creating some new tool for checking consistency of exceptions
> is not good way - and you are newer ensure consistency of custom
> exceptions.

Pointing out that some extension author, or pl/pgsql function, could
in principle ignore the documentation I'm asking you to write and not
supply a constraint, while raising their own, say, magical
ERRCODE_CHECK_VIOLATION is a bit of a cop-out. I should also mention
that things like ERRCODE_INTERNAL_ERROR are naturally going to be a
bit fuzzy, and that's fine. Nobody is ever going to expect those
anyway.

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

Right, or because you could do this and get an exception:

select 'foo'::bar_domain;

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

Can you give an example of when a generated name might be used, beyond
the example you've already given (that is, NULL constraints)? I'm not
completely opposed to the idea of a generated name. I think that it's
very much a secondary issue, though.

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

What does the option of following the SQL standard offer us? What have
I said that is fundamentally incompatible with how things work in this
patch right now?

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