On 9 May 2012 14:33, Pavel Stehule <pavel.steh...@gmail.com> wrote: > here is patch with enhancing ErrorData structure. Now constraints > errors and RI uses these fields
So I took a look at the patch eelog-2012-05-09.diff today. All of the following remarks apply to it alone. The patch has bitrotted some, due to the fact that Tom bashed around ri_triggers.c somewhat in recent weeks. I took the opportunity to resolve merge conflicts, and attach a revised patch for everyone's convenience. The regression tests pass when the patch is applied. The first thing I noticed about the patch was that inline functions are used freely. While I personally don't find this unreasonable, we recently revisited the question of whether or not it is necessary to continue to support compilers that do not support something equivalent to GNU C inline functions (or that cannot be made to support them via macro hacks). The outcome of that was that it remains necessary to use macros to provide non-inline equivalent functions. For a good example of how that was dealt with quite extensively, see the sortsupport commit: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c6e3ac11b60ac4a8942ab964252d51c1c0bd8845 In particular, take a look at the new file sortsupport.h . However, whatever we might some day allow with inline functions, the use of "extern inline", a C99 feature (or, according to some, anti-feature) seems like a nonstarter into the foreseeable future, unfortunately. Perhaps more to the point, are whatever performance benefit is to be had by the use of inline functions here actually going to pay for the maintenance cost? We already have functions declarations like this, in the same header as your proposed new extern inline functions: extern int geterrcode(void); This function's definition is trivial, and it would probably look like a good candidate for inlining to the compiler, if it had the chance (which, incidentally, it *still* may). However, why bother writing code to support (or encourage) this, considering that this is hardly a performance critical code-path, particularly unlikely to make any appreciable difference? Other style gripes: There are various points at which 80 columns are exceeded, contrary to our style guidelines. Sorry to have to mention it, but the comments need to be cleaned up (I am of course aware that English is not your first language, so that's not a big deal, and I don't think the content of the comments is lacking). The patch does essentially work as advertised. Sites that use the new infrastructure are limited to integrity constraint violations (which includes referential integrity constraint violations). So, based on your description, I'd have imagined that all of the sites using errcodes under this banner would have been covered: /* Class 23 - Integrity Constraint Violation */ This would be a reasonably well-defined place to require that this new infrastructure be used going forward (emphasis on the well-defined). Note that the authors of third-party database drivers define exception classes whose structure reflects these errcodes.h codes. To be inconsistent here seems unacceptable, since some future client of, say, pqxx (the example that I am personally most familiar with) might reasonably hope to always see some relation name when they call the e.relation_name() of some pqxx::integrity_constraint_violation object. If we were to commit the patch as-is, that would not be possible, because the following such sites that have not been touched: src/backend/commands/typecmds.c 2233: (errcode(ERRCODE_NOT_NULL_VIOLATION), src/backend/commands/tablecmds.c 3809: (errcode(ERRCODE_NOT_NULL_VIOLATION), src/backend/executor/execQual.c 3786: (errcode(ERRCODE_NOT_NULL_VIOLATION), src/backend/utils/adt/domains.c 126: (errcode(ERRCODE_NOT_NULL_VIOLATION), .... src/backend/utils/sort/tuplesort.c 3088: (errcode(ERRCODE_UNIQUE_VIOLATION), .... src/backend/commands/typecmds.c 2602: (errcode(ERRCODE_CHECK_VIOLATION), src/backend/commands/tablecmds.c 3823: (errcode(ERRCODE_CHECK_VIOLATION), 6633: (errcode(ERRCODE_CHECK_VIOLATION), src/backend/executor/execQual.c 3815: (errcode(ERRCODE_CHECK_VIOLATION), src/backend/utils/adt/domains.c 162: (errcode(ERRCODE_CHECK_VIOLATION), This function appears to be entirely vestigial, and can be removed, as it is never called: + extern inline int schema_table_column_error(const char *schema_name, const char *table_name, + const char *colname); This function is also vestigial and unused: + extern inline int rel_column_error(Oid table_oid, const char *colname); I have taken the liberty of removing both functions for you within the attached revision - I hope that's okay. Further gripes with the mechanism you've chosen: * Couldn't constraint_relation_error(Relation rel) just be implemented in terms of constraint_error(Relation rel, const char* cname)? * I doubt that relation_column_error() and friends belong in relcache.c at all, but if they do, then their prototypes belong in relcache.h, not rel.h * This seems rather broken to me: + static inline void + set_field(char **ptr, const char *str, bool overwrite) + { Why doesn't the function take "char *ptr" as its first argument? This looks like a modularity violation, since it would be perfectly possible to write the function as suggested. * Those functions use underscores rather than CamelCase, which is not consistent with the code that surround either the definitions or declarations. * ereport is used so frequently that it occurs to me that it would be nice to build some error-detection code into this expansion of the mechanism, to detect incorrect use (at least on debug configurations). So maybe constraint_relation_error() lives alongside errdetail() within elog.c. Maybe they return values of each function are some magic value, that is later read within errfinish(int dummy,...) via varargs. From there, on debug configurations, raise a warning if each argument is in a less than idiomatic order (so that we formalise the order). I'm not sure if that's worth the hassle of formalising, but it's worth considering, particularly as there are calls to make our error reporting mechanism more sophisticated. In the original thread on this (which was, regrettably, sidetracked by my tangential complaints about error severity), Robert expressed concerns about the performance impact of this patch, when plpgsql exception blocks were entered. I think it's a reasonable thing to be concerned about in general, and I'll address it when I address eelog-plpgsql-2012-05-09.diff separately. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services
eelog-2012-07-02.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
