On 12 October 2012 20:27, Pavel Stehule <pavel.steh...@gmail.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
