Re: enhanced error fields

On 9 May 2012 14:33, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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