Re: enhanced error fields

Hello Peter,

thank you very much for review

2012/7/2 Peter Geoghegan <peter(at)2ndquadrant(dot)com>:
> 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:

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