Hello Peter, thank you very much for review
2012/7/2 Peter Geoghegan <pe...@2ndquadrant.com>: > 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: using "inline" keyword is probably premature optimization in this context and better to remove it. This code is not on critical path. > > 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). it is ok. I'll reformat my code. > > 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), yes, it should be fixed > > 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. > No, it is not possible - I have to modify structure's fields - so I need addresses of pointers > * Those functions use underscores rather than CamelCase, which is not > consistent with the code that surround either the definitions or > declarations. It is hard question again - functions related to Relation usually use CamelCase, but functions related error processing use underscores - and I used it, because they used together with ereport. > > * 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. It is question. If I move constraint_relation_error to elog.c, then I have to include utils/rel.h to utils/elog.h. It was a issue previous version - criticised by Tom So current logic is - if you use "rel.h" and related structs, then you can set these fields in ErrorData. Regards Pavel > > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers