Re: enhanced error fields
От | Pavel Stehule |
---|---|
Тема | Re: enhanced error fields |
Дата | |
Msg-id | CAFj8pRAL8JT3aOK8gb=tFUSnWpsBChPJSaX1ndV0goqjKa4gTA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: enhanced error fields (Peter Geoghegan <peter@2ndquadrant.com>) |
Ответы |
Re: enhanced error fields
|
Список | pgsql-hackers |
Hello 2012/12/10 Peter Geoghegan <peter@2ndquadrant.com>: > So, I took a look at this, and made some more changes. > > I have a hard time seeing the utility of some fields that were in this > patch, and so they have been removed from this revision. > > Consider, for example: > > + constraint_table text, > + constraint_schema text, > > While constraint_name remains, I find it really hard to believe that > applications need a perfectly unambiguous idea of what constraint > they're dealing with. If applications have a constraint that has a > different domain-specific meaning depending on what schema it is in, > while a table in one schema uses that constraint in another, well, > that's a fairly awful design. Is it something that we really need to > care about? You mentioned the SQL standard, but as far as I know the > SQL standard has nothing to say about these fields within something > like errdata - their names are lifted from information_schema, but > being unambiguous is hardly so critical here. We want to identify an > object for the purposes of displaying an error message only. Caring > deeply about ambiguity seems wrong-headed to me in this context. > > + routine_name text, > + trigger_name text, > + trigger_table text, > + trigger_schema text, > > The whole point of an exception (which ereport() is very similar to) > is to decouple errors from error handling as much as possible - any > application that magically takes a different course of action based on > where that error occurred as reported by the error itself (and not the > location of where handling the error occurs) seems kind of > wrong-headed to me. Sure, a backtrace may be supplied, but that's only > for diagnostic purposes, and a human can just as easily get that > information already. If you can present an example of this information > actually being present in a way that is amenable to that, either in > any other RDBMS or any major programming language or framework, I > might reconsider. > > This just leaves: > > + column_name text, > + table_name text, > + constraint_name text, > + schema_name text, I agree so lot of mentioned fields are difficult defined in PostgreSQL environment due different meaning between ANSI SQL and PostgreSQL and because PostgreSQL doesn't support some ANSI SQL features - assertions. There are two basic aspects of design * usability - we would to remove necessity of parsing error messages for getting interesting data, it decrease dependency on current error messages - then I am thinking so some informations about triggers or functions where exception was born are practical (current implementation is different task - we can find better solution than I proposed highly probably) * compatibility - personally, lot of diagnostics fields are very vague defined, so here can be lot of issues - I have no experience with DB2 or Oracle, so I cannot to speak more, but we should to say, what we expect. > > This seems like enough information for any reasonable use of this > infrastructure, and I highly doubt anyone would avail of the other > fields if they remained. I guess an application might have done > something with "constraint_table", as with foreign keys for example, > but I just can't see that being used when constraint_name can be used. > I afraid so constraint name is not afraid postgres=# create table a (a int primary key); CREATE TABLE postgres=# create table b (b int references a(a)); CREATE TABLE postgres=# insert into b values (10); ERROR: insert or update on table "b" violates foreign key constraint "b_b_fkey" DETAIL: Key (b)=(10) is not present in table "a". postgres=# \set VERBOSITY verbose postgres=# insert into b values (10); ERROR: 23503: insert or update on table "b" violates foreign key constraint "b_b_fkey" DETAIL: Key (b)=(10) is not present in table "a". LOCATION: ri_ReportViolation, ri_triggers.c:3222 postgres=# insert into a values(10); INSERT 0 1 postgres=# insert into b values (10); INSERT 0 1 postgres=# delete from a; ERROR: 23503: update or delete on table "a" violates foreign key constraint "b_b_fkey" on table "b" DETAIL: Key (a)=(10) is still referenced from table "b". LOCATION: ri_ReportViolation, ri_triggers.c:3232 there are two different bugs - with same ERRCODE and constraint name - once is problem on "b", second on "a" so constraint_table is interesting information > Removing these fields has also allowed me to remove the "avoid setting > field at lower point in the callstack" logic, which was itself kind of > ugly. Since fields like routine_name only set themselves at the top of > the callstack, the potential for astonishing outcomes was pretty high > - what if you expect one routine_name, but then that routine follows a > slightly different code-path one day and you get another function > setting routine_name and undermining that expectation? > My implementation is probably too dumb - but a question - "where exception is coming from?" is relative typical - but we can to clean implementation. I see a issue in definition. Usually I am interesting about most deep custom code - not most deep code. so I am not against to removing some fields, but constraint_table and routine_name is useful and we should to produce this information. > There were some bugs left in the patch eelog3.diff, mostly due to > things like setting table name to what is actually an index name. As I > mentioned, we now assert that: > > + Assert(table->rd_rel->relkind == RELKIND_RELATION); > > in the functions within relerror.c. > > I have tightened up where these fields are available, and > appropriately documented that for the benefit of both application > developers and developers of client libraries. I went so far as to > hack the Perl scripts that generate .sgml and .h files from > errcodes.txt (i.e. the infrastrucutre that produces tables of errcodes > in various places from a single authoritative place) - I have > instituted a coding standard so that these fields are reliably > available and have documented that requirement at both the user and > hacker level. > > It would be nice if a Perl hacker could eyeball those changes - this > is my first time writing Perl, and I suspect it may be worth having > someone else to polish the Perl code a bit. > > I have emphasized the need for consistency and a sane contract for > application developers and third-party client driver authors - they > *need* to know that certain fields will always be available, or at > least won't be unavailable due to a change in the phase of the moon. > > errcodes.txt now says: > > + # Postgres coding standards mandate that certain fields be available in all > + # instances for some of the Class 23 errcodes, documented under > "Requirement: " > + # here. Some other errcode's ereport sites may, at their own discretion, make > + # errcolumn, errtable, errconstraint and errschema fields available too. > + # Furthermore, it is possible to make some fields available beyond those > + # formally required at callsites involving these Class 23 errcodes with > + # "Requirements: ". > Section: Class 23 - Integrity Constraint Violation > ! Requirement: unused > 23000 E ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION > integrity_constraint_violation > + Requirement: unused > 23001 E ERRCODE_RESTRICT_VIOLATION > restrict_violation > + # Note that requirements for ERRCODE_NOT_NULL do not apply to domains: > + Requirement: schema_name, table_name > 23502 E ERRCODE_NOT_NULL_VIOLATION > not_null_violation > + Requirement: schema_name, table_name, constraint_name > 23503 E ERRCODE_FOREIGN_KEY_VIOLATION > foreign_key_violation > + Requirement: schema_name, table_name, constraint_name > 23505 E ERRCODE_UNIQUE_VIOLATION > unique_violation > + Requirement: constraint_name > 23514 E ERRCODE_CHECK_VIOLATION > check_violation > + Requirement: schema_name, table_name, constraint_name > 23P01 E ERRCODE_EXCLUSION_VIOLATION > exclusion_violation > > Now, there are one or two places where these fields are not actually > available even though they're formally required according to a literal > reading of the above. This is only because there is clearly no such > field sensibly available, even in principle - to my mind this cannot > be a problem, because the application developer cannot have any > reasonable expectation of a field being set. I'm really talking about > two cases in particular: > > * For ERRCODE_NOT_NULL_VIOLATION, we don't actually provide > schema_name and table_name in the event of domains. This was > previously identified as an issue. If it is judged better to not have > any requirements there at all, so be it. > > * For the validateDomainConstraint() ERRCODE_CHECK_VIOLATION ereport > call, we may not provide a constraint name iff a Constraint.connname > is NULL. Since there isn't a constraint name to give even in > principle, and this is an isolated case, this seems reasonable. ok > > To make logging less verbose, TABLE NAME isn't consistently split out > as a separate field - this seems fine to me, since application code > doesn't target logs: > > + if (edata->column_name && edata->table_name) > + { > + log_line_prefix(&buf, edata); > + appendStringInfo(&buf, _("COLUMN NAME: %s:%s\n"), > + edata->table_name, edata->column_name); > + } > + else if (edata->table_name) > + { > + log_line_prefix(&buf, edata); > + appendStringInfo(&buf, _("TABLE NAME: %s\n"), > + edata->table_name); > + } uff, no - I don't like it - it is not consistent and I don't think so one row more is a issue. But is a question if we don't need a second "VERBOSE" level for showing these fields - maybe VERBOSE_ENHANCED or some similar we don't need log these fields usually ?? > > I used pgindent to selectively indent parts of the codebase affected > by this patch. I am about ready to mark this one "ready for > committer", but it would be nice at this point to get some buy-in on > the basic view of how these things should work that informed this > revision. Does anyone disagree with my contention that there should be > a sane, well-defined contract, or any of the details of what that > should look like? Was I right to suggest that some of the set of > fields that appeared in Pavel's eelog3.diff revision are unnecessary? > > I'm sorry it took me as long as it did to get back to you on this. > It is ok, thank you very much Regards Pavel > -- > Peter Geoghegan http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training and Services
В списке pgsql-hackers по дате отправления: