enhanced error fields

On Wed, May 9, 2012 at 9:33 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:

here is patch with enhancing ErrorData structure. Now constraints
errors and RI uses these fields

Please add this to https://commitfest.postgresql.org/action/commitfest_view/open

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 9 May 2012 14:33, Pavel Stehule <pavel.stehule@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

On 2 July 2012 15:19, Peter Geoghegan <peter@2ndquadrant.com> wrote:

On 9 May 2012 14:33, Pavel Stehule <pavel.stehule@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.

I decided to follow through and take a look at
eelog-plpgsql-2012-05-09.diff today too, while I have all of this
swapped into my head.

This patch is not an atomic unit - it builds upon the first patch. I
successfully merged the local feature branch that I'd created for
eelog-2012-05-09.diff without any merge conflicts, and I can build
Postgres and get the regression tests to pass (including a couple of
new ones, for this added functionality for plggsql - the functionality
is testing exclusively using the new (9.2) "get stacked diagnostics"
and "raise custom exception 'some_custom_exception' using...."
feature).

Since that feature branch had all my revisions committed, my
observations about redundancies in the other base patch still stand -
the 2 functions mentioned did not exist for the benefit of this
further patch either.

There is a typo here:

+ 				case PLPGSQL_RAISEOPTION_TRIGGER_SCHEMA:
+ 					printf("    TRIGGER_SCHENA = ");
+ 					break;
  			}

I'm not sure about this inconsistency within unreserved_keyword:

For routines:
+ 				| K_DIAG_ROUTINE_NAME
+ 				| K_DIAG_ROUTINE_SCHEMA
....

For triggers:
+ 				| K_DIAG_TRIGGER_NAME
+ 				| K_DIAG_TRIGGER_SCHEMA
....

For tables:
+ | K_DIAG_SCHEMA_NAME
.
. **SNIP**
.
+ | K_DIAG_TABLE_NAME

The same inconsistency exists within the anonymous enum that contains
PLPGSQL_GETDIAG_TABLE_NAME (and other constants), as well as the new
token keywords within plpgsql's gram.y .

The doc changes need a little work here too.

I'm not sure that I agree with the extensive use of the term "routine"
in all of these constants - sure, information_schema has a view called
"routines". But wouldn't it be more appropriate to use a
Postgres-centric term within our own code?

So, what about the concern about performance taking a hit when plpgsql
exception blocks are entered as a result of this patch? Well, while I
think that an effort to reduce the overhead of PL exception handling
would be worthwhile, these patches do not appear to alter things
appreciable (though the overhead *is* measurable):

[peter@peterlaptop eelog]$ ls
exceptions.sql test_eelog_outer.sql

Patch (eelog-plpgsql):

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 305756
tps = 1019.026055 (including connections establishing)
tps = 1019.090135 (excluding connections establishing)

Master:

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 308376
tps = 1027.908182 (including connections establishing)
tps = 1027.977879 (excluding connections establishing)

An archive with simple scripts for repeating this are attached, if
anyone is interested.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

2012/7/2 Peter Geoghegan <peter@2ndquadrant.com>:

On 2 July 2012 15:19, Peter Geoghegan <peter@2ndquadrant.com> wrote:

On 9 May 2012 14:33, Pavel Stehule <pavel.stehule@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.

I decided to follow through and take a look at
eelog-plpgsql-2012-05-09.diff today too, while I have all of this
swapped into my head.

This patch is not an atomic unit - it builds upon the first patch. I
successfully merged the local feature branch that I'd created for
eelog-2012-05-09.diff without any merge conflicts, and I can build
Postgres and get the regression tests to pass (including a couple of
new ones, for this added functionality for plggsql - the functionality
is testing exclusively using the new (9.2) "get stacked diagnostics"
and "raise custom exception 'some_custom_exception' using...."
feature).

Since that feature branch had all my revisions committed, my
observations about redundancies in the other base patch still stand -
the 2 functions mentioned did not exist for the benefit of this
further patch either.

There is a typo here:

+                               case PLPGSQL_RAISEOPTION_TRIGGER_SCHEMA:
+                                       printf("    TRIGGER_SCHENA = ");
+                                       break;
                       }

I'm not sure about this inconsistency within unreserved_keyword:

For routines:
+                               | K_DIAG_ROUTINE_NAME
+                               | K_DIAG_ROUTINE_SCHEMA
....

For triggers:
+                               | K_DIAG_TRIGGER_NAME
+                               | K_DIAG_TRIGGER_SCHEMA
....

For tables:
+                               | K_DIAG_SCHEMA_NAME
.
. **SNIP**
.
+                               | K_DIAG_TABLE_NAME

The same inconsistency exists within the anonymous enum that contains
PLPGSQL_GETDIAG_TABLE_NAME (and other constants), as well as the new
token keywords within plpgsql's gram.y .

The doc changes need a little work here too.

I'm not sure that I agree with the extensive use of the term "routine"
in all of these constants - sure, information_schema has a view called
"routines". But wouldn't it be more appropriate to use a
Postgres-centric term within our own code?

So, what about the concern about performance taking a hit when plpgsql
exception blocks are entered as a result of this patch? Well, while I
think that  an effort to reduce the overhead of PL exception handling
would be worthwhile, these patches do not appear to alter things
appreciable (though the overhead *is* measurable):

[peter@peterlaptop eelog]$ ls
exceptions.sql  test_eelog_outer.sql

Patch (eelog-plpgsql):

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 305756
tps = 1019.026055 (including connections establishing)
tps = 1019.090135 (excluding connections establishing)

Master:

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 308376
tps = 1027.908182 (including connections establishing)
tps = 1027.977879 (excluding connections establishing)

An archive with simple scripts for repeating this are attached, if
anyone is interested.

yes, I think so slowdown is not significant for usual situations and
patterns. I got about 3% slowdown on code like

begin
insert into tab -- raise exception
exception when others ..
...
end;

and only when all inserts fails.

Regards

Pavel

Show quoted text

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Hello Peter,

thank you very much for review

2012/7/2 Peter Geoghegan <peter@2ndquadrant.com>:

On 9 May 2012 14:33, Pavel Stehule <pavel.stehule@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

Show quoted text

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

2012/7/2 Peter Geoghegan <peter@2ndquadrant.com>:

On 2 July 2012 15:19, Peter Geoghegan <peter@2ndquadrant.com> wrote:

On 9 May 2012 14:33, Pavel Stehule <pavel.stehule@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.

I decided to follow through and take a look at
eelog-plpgsql-2012-05-09.diff today too, while I have all of this
swapped into my head.

This patch is not an atomic unit - it builds upon the first patch. I
successfully merged the local feature branch that I'd created for
eelog-2012-05-09.diff without any merge conflicts, and I can build
Postgres and get the regression tests to pass (including a couple of
new ones, for this added functionality for plggsql - the functionality
is testing exclusively using the new (9.2) "get stacked diagnostics"
and "raise custom exception 'some_custom_exception' using...."
feature).

Since that feature branch had all my revisions committed, my
observations about redundancies in the other base patch still stand -
the 2 functions mentioned did not exist for the benefit of this
further patch either.

There is a typo here:

+                               case PLPGSQL_RAISEOPTION_TRIGGER_SCHEMA:
+                                       printf("    TRIGGER_SCHENA = ");
+                                       break;
                       }

I'm not sure about this inconsistency within unreserved_keyword:

For routines:
+                               | K_DIAG_ROUTINE_NAME
+                               | K_DIAG_ROUTINE_SCHEMA
....

For triggers:
+                               | K_DIAG_TRIGGER_NAME
+                               | K_DIAG_TRIGGER_SCHEMA
....

For tables:
+                               | K_DIAG_SCHEMA_NAME
.
. **SNIP**
.
+                               | K_DIAG_TABLE_NAME

This inconsistency is based on ANSI SQL design - we can use own
identifiers, but I prefer identifiers based on keyword strings.

The same inconsistency exists within the anonymous enum that contains
PLPGSQL_GETDIAG_TABLE_NAME (and other constants), as well as the new
token keywords within plpgsql's gram.y .

see standard, please :) - it is not consistent. But it use short and
clean names, and it is relative widely used.

The doc changes need a little work here too.

please, if you can enhance documentation, please, do it. I am not
native speaker.

I'm not sure that I agree with the extensive use of the term "routine"
in all of these constants - sure, information_schema has a view called
"routines". But wouldn't it be more appropriate to use a
Postgres-centric term within our own code?

for me - routine is general world for functions and procedures. So
using routine in PostgreSQL is ok. And I believe so we will support
procedures too some day.

So, what about the concern about performance taking a hit when plpgsql
exception blocks are entered as a result of this patch? Well, while I
think that  an effort to reduce the overhead of PL exception handling
would be worthwhile, these patches do not appear to alter things
appreciable (though the overhead *is* measurable):

[peter@peterlaptop eelog]$ ls
exceptions.sql  test_eelog_outer.sql

Patch (eelog-plpgsql):

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 305756
tps = 1019.026055 (including connections establishing)
tps = 1019.090135 (excluding connections establishing)

Master:

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 308376
tps = 1027.908182 (including connections establishing)
tps = 1027.977879 (excluding connections establishing)

An archive with simple scripts for repeating this are attached, if
anyone is interested.

thank you for performance testing. It verify my own testing.

Thank you for review,

Regards

Pavel

Show quoted text

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Hi,

On Monday, July 02, 2012 04:19:56 PM Peter Geoghegan wrote:

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=c6e3ac11b60ac
4a8942ab964252d51c1c0bd8845

In particular, take a look at the new file sortsupport.h .

I actually find sortsupport.h not to be a good example in general because it
duplicates the code. Unless youre dealing with minor amounts of code playing
macro tricks like I did in the ilist stuff seems to be a better idea.

Greetings,

Andres
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Excerpts from Pavel Stehule's message of mar jul 03 12:26:57 -0400 2012:

2012/7/2 Peter Geoghegan <peter@2ndquadrant.com>:

* 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

Including rel.h in elog.h is really really bad. Even if it was only
relcache.h it would be bad, because elog.h is included *everywhere*.

So current logic is - if you use "rel.h" and related structs, then you
can set these fields in ErrorData.

Hm. These new functions do not operate on Relations at all, so having
them on relcache.c doesn't seem to me very good ...

How about putting the functions in elog.c as Peter suggests, and the
declarations in a new header (say relationerror.h or something like
that)? That new header would #include relcache.h so if you need to set
those fields you include the new header and you have everything you
need.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2012/7/3 Alvaro Herrera <alvherre@commandprompt.com>:

Excerpts from Pavel Stehule's message of mar jul 03 12:26:57 -0400 2012:

2012/7/2 Peter Geoghegan <peter@2ndquadrant.com>:

* 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

Including rel.h in elog.h is really really bad.  Even if it was only
relcache.h it would be bad, because elog.h is included *everywhere*.

So current logic is - if you use "rel.h" and related structs, then you
can set these fields in ErrorData.

Hm.  These new functions do not operate on Relations at all, so having
them on relcache.c doesn't seem to me very good ...

How about putting the functions in elog.c as Peter suggests, and the
declarations in a new header (say relationerror.h or something like
that)?  That new header would #include relcache.h so if you need to set
those fields you include the new header and you have everything you
need.

it could be

Pavel

Show quoted text

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 3 July 2012 17:26, Pavel Stehule <pavel.stehule@gmail.com> wrote:

Hello Peter,

thank you very much for review

No problem.

I'll do some copy-editing of comments and doc changes when you produce
another revision.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Peter Geoghegan <peter@2ndquadrant.com> writes:

So I took a look at the patch eelog-2012-05-09.diff today. All of the
following remarks apply to it alone.

I've been trying out this patch for my own interest (I'm very pleased to
see work on this feature), and I have a couple of suggestions from a
user's point of view.

First: if a not null constraint is violated, the error report includes
CONSTRAINT NAME 'not_null_violation'. I think I would find it more
useful if CONSTRAINT NAME were left unset rather than given a value that
doesn't correspond to a real constraint. A client program can tell it's
a null constraint violation from the SQLSTATE.

Second: in the case where a foreign-key constraint is violated by a
change in the primary-key table, the error report gives the following
information:

TABLE NAME: name of primary-key table
SCHEMA NAME: schema of primary-key table
CONSTRAINT NAME: name of foreign-key constraint
CONSTRAINT SCHEMA: schema of foreign-key table

It doesn't include the name of the foreign-key table (except in the
human-readable error message). But in principle you need to know that
table name to reliably identify the constraint that was violated.

I think what's going on is a mismatch between the way the constraint
namespace works in the SQL standard and in PostgreSQL: it looks like the
standard expects constraint names to be unique within a schema, while
PostgreSQL only requires them to be unique within a table. (A similar
issue makes information_schema less useful than the pg_ tables for
foreign key constraints.)

So I think it would be helpful to go beyond the standard in this case
and include the foreign-key table name somewhere in the report.

Possibly the enhanced-error reports could somehow add the table name to
the string in the CONSTRAINT NAME field, so that the interface
PostgreSQL provides looks like the one the standard envisages (which
ought to make it easier to write cross-database client code).

Or it might be simpler to just add a new enhanced-error field; I can
imagine cases where that table name would be the main thing I'd be
interested in.

-M-

Hello

2012/7/3 Matthew Woodcraft <matthew@woodcraft.me.uk>:

Peter Geoghegan <peter@2ndquadrant.com> writes:

So I took a look at the patch eelog-2012-05-09.diff today. All of the
following remarks apply to it alone.

I've been trying out this patch for my own interest (I'm very pleased to
see work on this feature), and I have a couple of suggestions from a
user's point of view.

First: if a not null constraint is violated, the error report includes
CONSTRAINT NAME 'not_null_violation'. I think I would find it more
useful if CONSTRAINT NAME were left unset rather than given a value that
doesn't correspond to a real constraint. A client program can tell it's
a null constraint violation from the SQLSTATE.

I don't think so generation some special name is good idea. In this
case - important values are in COLUMN_NAME, TABLE_NAME, SCHEMA_NAME

postgres=# create table ff(a int not null);
CREATE TABLE
postgres=# \set VERBOSITY verbose
postgres=# insert into ff values(null);
ERROR: 23502: null value in column "a" violates not-null constraint
DETAIL: Failing row contains (null).
LOCATION: ExecConstraints, execMain.c:1527
COLUMN NAME: a
TABLE NAME: ff
SCHEMA NAME: public
CONSTRAINT NAME: not_null_violation
CONSTRAINT SCHEMA: public

Second: in the case where a foreign-key constraint is violated by a
change in the primary-key table, the error report gives the following
information:

  TABLE NAME:        name of primary-key table
  SCHEMA NAME:       schema of primary-key table
  CONSTRAINT NAME:   name of foreign-key constraint
  CONSTRAINT SCHEMA: schema of foreign-key table

postgres=# create table a1(a int primary key);
NOTICE: 00000: CREATE TABLE / PRIMARY KEY will create implicit index
"a1_pkey" for table "a1"
LOCATION: DefineIndex, indexcmds.c:600
CREATE TABLE
postgres=# create table a2(a int references a1(a));
CREATE TABLE
postgres=# insert into a2 values(10);
ERROR: 23503: insert or update on table "a2" violates foreign key
constraint "a2_a_fkey"
DETAIL: Key (a)=(10) is not present in table "a1".
LOCATION: ri_ReportViolation, ri_triggers.c:3228
TABLE NAME: a2
SCHEMA NAME: public
CONSTRAINT NAME: a2_a_fkey
CONSTRAINT SCHEMA: public

postgres=# \d a2
Table "public.a2"
Column │ Type │ Modifiers
────────┼─────────┼───────────
a │ integer │
Foreign-key constraints:
"a2_a_fkey" FOREIGN KEY (a) REFERENCES a1(a)

I agree so access to related table is not simple, but you know
constraint name, and you can take referenced table from constraint
definition.

so any special column is not necessary. It can be done in future, but
for this moment I would add only really necessary fields.

It doesn't include the name of the foreign-key table (except in the
human-readable error message). But in principle you need to know that
table name to reliably identify the constraint that was violated.

I think what's going on is a mismatch between the way the constraint
namespace works in the SQL standard and in PostgreSQL: it looks like the
standard expects constraint names to be unique within a schema, while
PostgreSQL only requires them to be unique within a table. (A similar
issue makes information_schema less useful than the pg_ tables for
foreign key constraints.)

So I think it would be helpful to go beyond the standard in this case
and include the foreign-key table name somewhere in the report.

Possibly the enhanced-error reports could somehow add the table name to
the string in the CONSTRAINT NAME field, so that the interface
PostgreSQL provides looks like the one the standard envisages (which
ought to make it easier to write cross-database client code).

same situation is with triggers

I prefer add two new fields CONSTRAINT_TABLE and TRIGGER_TABLE so
NAME, TABLE and SCHEMA is unique

Regards and thank you for comments

Pavel

Show quoted text

Or it might be simpler to just add a new enhanced-error field; I can
imagine cases where that table name would be the main thing I'd be
interested in.

-M-

Excerpts from Pavel Stehule's message of mié jul 04 05:33:48 -0400 2012:

Hello

2012/7/3 Matthew Woodcraft <matthew@woodcraft.me.uk>:

Peter Geoghegan <peter@2ndquadrant.com> writes:

So I took a look at the patch eelog-2012-05-09.diff today. All of the
following remarks apply to it alone.

I've been trying out this patch for my own interest (I'm very pleased to
see work on this feature), and I have a couple of suggestions from a
user's point of view.

First: if a not null constraint is violated, the error report includes
CONSTRAINT NAME 'not_null_violation'. I think I would find it more
useful if CONSTRAINT NAME were left unset rather than given a value that
doesn't correspond to a real constraint. A client program can tell it's
a null constraint violation from the SQLSTATE.

I don't think so generation some special name is good idea. In this
case - important values are in COLUMN_NAME, TABLE_NAME, SCHEMA_NAME

postgres=# create table ff(a int not null);
CREATE TABLE
postgres=# \set VERBOSITY verbose
postgres=# insert into ff values(null);
ERROR: 23502: null value in column "a" violates not-null constraint
DETAIL: Failing row contains (null).
LOCATION: ExecConstraints, execMain.c:1527
COLUMN NAME: a
TABLE NAME: ff
SCHEMA NAME: public
CONSTRAINT NAME: not_null_violation
CONSTRAINT SCHEMA: public

I think if you don't have a true constraint name to use here, you
shouldn't use anything. When and if we get NOT NULL constraints
catalogued, we can add a constraint name field as a new error field.

In other words +1 for Matthew's opinion.

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:

I think if you don't have a true constraint name to use here, you
shouldn't use anything.

Yeah, I agree. Don't invent a value, just omit the field.

regards, tom lane

2012/7/4 Tom Lane <tgl@sss.pgh.pa.us>:

Alvaro Herrera <alvherre@commandprompt.com> writes:

I think if you don't have a true constraint name to use here, you
shouldn't use anything.

Yeah, I agree.  Don't invent a value, just omit the field.

ok

Pavel

Show quoted text

                        regards, tom lane

Hello

there is a updated patch:

* renamed auxiliary functions and moved it elog.c - header is new file
"relerror.h"
* new fields "constraint_table" and "trigger_table" - constraints and
triggers are related to relation in pg, not just to schema
* removed using implicit constraints without unique name
* better coverage of enhancing errors in source code
* removed "inline" keywords

/* 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/executor/execQual.c
3786: (errcode(ERRCODE_NOT_NULL_VIOLATION),

src/backend/utils/adt/domains.c
126: (errcode(ERRCODE_NOT_NULL_VIOLATION),

src/backend/executor/execQual.c
3815: (errcode(ERRCODE_CHECK_VIOLATION),

src/backend/utils/adt/domains.c
162: (errcode(ERRCODE_CHECK_VIOLATION),

these exceptions are related to domains - we has not adequate fields
now - and these fields are not in standards

it needs some like DOMAIN_NAME and DOMAIN_SCHEMA ???

Regards

Pavel

Hello

* fixed typo
* support two new fields: constraint_table and trigger_table
* routine_table, routine_schema, trigger_name, trigger_table,
trigger_schema has value when exception coming from plpgsql.

Regards

Pavel

2012/7/2 Peter Geoghegan <peter@2ndquadrant.com>:

Show quoted text

On 2 July 2012 15:19, Peter Geoghegan <peter@2ndquadrant.com> wrote:

On 9 May 2012 14:33, Pavel Stehule <pavel.stehule@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.

I decided to follow through and take a look at
eelog-plpgsql-2012-05-09.diff today too, while I have all of this
swapped into my head.

This patch is not an atomic unit - it builds upon the first patch. I
successfully merged the local feature branch that I'd created for
eelog-2012-05-09.diff without any merge conflicts, and I can build
Postgres and get the regression tests to pass (including a couple of
new ones, for this added functionality for plggsql - the functionality
is testing exclusively using the new (9.2) "get stacked diagnostics"
and "raise custom exception 'some_custom_exception' using...."
feature).

Since that feature branch had all my revisions committed, my
observations about redundancies in the other base patch still stand -
the 2 functions mentioned did not exist for the benefit of this
further patch either.

There is a typo here:

+                               case PLPGSQL_RAISEOPTION_TRIGGER_SCHEMA:
+                                       printf("    TRIGGER_SCHENA = ");
+                                       break;
}

I'm not sure about this inconsistency within unreserved_keyword:

For routines:
+                               | K_DIAG_ROUTINE_NAME
+                               | K_DIAG_ROUTINE_SCHEMA
....

For triggers:
+                               | K_DIAG_TRIGGER_NAME
+                               | K_DIAG_TRIGGER_SCHEMA
....

For tables:
+                               | K_DIAG_SCHEMA_NAME
.
. **SNIP**
.
+                               | K_DIAG_TABLE_NAME

The same inconsistency exists within the anonymous enum that contains
PLPGSQL_GETDIAG_TABLE_NAME (and other constants), as well as the new
token keywords within plpgsql's gram.y .

The doc changes need a little work here too.

I'm not sure that I agree with the extensive use of the term "routine"
in all of these constants - sure, information_schema has a view called
"routines". But wouldn't it be more appropriate to use a
Postgres-centric term within our own code?

So, what about the concern about performance taking a hit when plpgsql
exception blocks are entered as a result of this patch? Well, while I
think that an effort to reduce the overhead of PL exception handling
would be worthwhile, these patches do not appear to alter things
appreciable (though the overhead *is* measurable):

[peter@peterlaptop eelog]$ ls
exceptions.sql test_eelog_outer.sql

Patch (eelog-plpgsql):

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 305756
tps = 1019.026055 (including connections establishing)
tps = 1019.090135 (excluding connections establishing)

Master:

[peter@peterlaptop eelog]$ pgbench -T 300 -f exceptions.sql -c 10 -n
transaction type: Custom query
scaling factor: 1
query mode: simple
number of clients: 10
number of threads: 1
duration: 300 s
number of transactions actually processed: 308376
tps = 1027.908182 (including connections establishing)
tps = 1027.977879 (excluding connections establishing)

An archive with simple scripts for repeating this are attached, if
anyone is interested.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

Attached is a revision of this patch, with a few clean-ups, mostly to
the wording of certain things.

On 5 July 2012 17:41, Pavel Stehule <pavel.stehule@gmail.com> wrote:

* renamed auxiliary functions and moved it elog.c - header is new file
"relerror.h"

In my revision, I've just added a pre-declaration and removed the
dedicated header, which didn't make too much sense to me:

+ /* Pre-declare Relation, in order to avoid a build dependency on rel.h. */
+ typedef struct RelationData *Relation;

This works fine, and is precedented. See
src/include/storage/buffile.h, for example. If you think it's
unreasonable that none of the functions now added to elog.h are
callable without also including rel.h, consider that all call sites
are naturally already doing that, for the errmsg() string itself.
Besides, this is a perfectly valid way of reducing build dependencies,
or at least a more valid way than creating a new header that does not
really represent a natural new division for these new functions, IMHO.
Opaque pointers are ordinarily used to encapsulate things in C, rather
than to prevent build dependencies, but I believe that's only because
in general that's something that C programmers are more likely to
want.

* new fields "constraint_table" and "trigger_table" - constraints and
triggers are related to relation in pg, not just to schema

I've added some remarks to that effect in the docs of my revision for
your consideration.

* better coverage of enhancing errors in source code

Good. I think it's important that we nail down just where these are
expected to be available. It would be nice if there was a quick and
easy answer to the question "Just what ErrorResponse fields should
this new "sub-category of class 23" ereport() site have?". We clearly
have yet to work those details out.

I have another concern with this patch. log_error_verbosity appears to
be intended as an exact analogue of client verbosity (as set by
PQsetErrorVerbosity()). While this generally holds for this patch,
there is one notable exception: You always log all of these new fields
within write_csvlog(), even if (Log_error_verbosity <
PGERROR_VERBOSE). Why? There will be a bunch of commas in most CSV
logs once this happens, so that the schema of the log is consistent.
That is kind of ugly, but I don't see a way around it. We already do
this for location and application_name (though that's separately
controlled by the application_name guc). I haven't touched that in the
attached revision, as I'd like to hear what you have to say.

Another problem that will need to be fixed is that of the follow values:

+#define PG_DIAG_COLUMN_NAME	'c'
+#define PG_DIAG_TABLE_NAME	't'
+#define PG_DIAG_SCHEMA_NAME	's'
+#define PG_DIAG_CONSTRAINT_NAME		'n'
+#define PG_DIAG_CONSTRAINT_TABLE	'o'
+#define PG_DIAG_CONSTRAINT_SCHEMA	'm'
+#define PG_DIAG_ROUTINE_NAME		'r'
+#define PG_DIAG_ROUTINE_SCHEMA		'u'
+#define PG_DIAG_TRIGGER_NAME		'g'
+#define PG_DIAG_TRIGGER_TABLE		'i'
+#define PG_DIAG_TRIGGER_SCHEMA		'h'

Not all appear to have a way of setting the value within the ereport
interface. For example, there is nothing like "errrelation_column()"
(or "errrelcol()", as I call it) to set PG_DIAG_ROUTINE_NAME. This is
something I haven't touched.

src/backend/utils/adt/domains.c
162: (errcode(ERRCODE_CHECK_VIOLATION),

these exceptions are related to domains - we has not adequate fields
now - and these fields are not in standards

it needs some like DOMAIN_NAME and DOMAIN_SCHEMA ???

Hmm. I'm not sure that it's worth it.

I took a look at recent JDBC documentation, because I'd expect it to
offer the most complete support for exposing this in exception
classes. Turns out that it does not expose things at as fine a
granularity as you have here at all, which is disappointing. It seems
to suppose that just a vendor code and "cause" will be sufficient. If
you're using this one proprietary database, there is a command that'll
let you get diagnostics. The wisdom for users of another proprietary
database seems to be that you just use stored procedures. So I agree
that CONSTRAINT NAME should be unset in the event of uncatalogued,
unnamed not null integrity constraint violations. The standard seems
to be loose on this, and I think we'll have to be too. However, I'd
find it pretty intolerable if we were inconsistent between ereport()
callsites that were *effectively the same error* - this could result
in a user's application breaking based on the phase of the moon.

Do you suppose it's worth stashing the last set of these fields to one
side, and exposing this through an SQL utility command, or even a
bundled function? I don't imagine that it's essential to have that
right away, but it's something to consider.

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

2012/7/7 Peter Geoghegan <peter@2ndquadrant.com>:

Attached is a revision of this patch, with a few clean-ups, mostly to
the wording of certain things.

On 5 July 2012 17:41, Pavel Stehule <pavel.stehule@gmail.com> wrote:

* renamed auxiliary functions and moved it elog.c - header is new file
"relerror.h"

In my revision, I've just added a pre-declaration and removed the
dedicated header, which didn't make too much sense to me:

+ /* Pre-declare Relation, in order to avoid a build dependency on rel.h. */
+ typedef struct RelationData *Relation;

This works fine, and is precedented. See
src/include/storage/buffile.h, for example. If you think it's
unreasonable that none of the functions now added to elog.h are
callable without also including rel.h, consider that all call sites
are naturally already doing that, for the errmsg() string itself.
Besides, this is a perfectly valid way of reducing build dependencies,
or at least a more valid way than creating a new header that does not
really represent a natural new division for these new functions, IMHO.
Opaque pointers are ordinarily used to encapsulate things in C, rather
than to prevent build dependencies, but I believe that's only because
in general that's something that C programmers are more likely to
want.

It is question for Alvaro or Tom. I have not strong opinion on it.

* new fields "constraint_table" and "trigger_table" - constraints and
triggers are related to relation in pg, not just to schema

I've added some remarks to that effect in the docs of my revision for
your consideration.

* better coverage of enhancing errors in source code

Good. I think it's important that we nail down just where these are
expected to be available. It would be nice if there was a quick and
easy answer to the question "Just what ErrorResponse fields should
this new "sub-category of class 23" ereport() site have?". We clearly
have yet to work those details out.

I have another concern with this patch. log_error_verbosity appears to
be intended as an exact analogue of client verbosity (as set by
PQsetErrorVerbosity()). While this generally holds for this patch,
there is one notable exception: You always log all of these new fields
within write_csvlog(), even if (Log_error_verbosity <
PGERROR_VERBOSE). Why? There will be a bunch of commas in most CSV
logs once this happens, so that the schema of the log is consistent.
That is kind of ugly, but I don't see a way around it. We already do
this for location and application_name (though that's separately
controlled by the application_name guc). I haven't touched that in the
attached revision, as I'd like to hear what you have to say.

it is bug - these new fields should be used only when verbosity is >= VERBOSE

Another problem that will need to be fixed is that of the follow values:

+#define PG_DIAG_COLUMN_NAME    'c'
+#define PG_DIAG_TABLE_NAME     't'
+#define PG_DIAG_SCHEMA_NAME    's'
+#define PG_DIAG_CONSTRAINT_NAME                'n'
+#define PG_DIAG_CONSTRAINT_TABLE       'o'
+#define PG_DIAG_CONSTRAINT_SCHEMA      'm'
+#define PG_DIAG_ROUTINE_NAME           'r'
+#define PG_DIAG_ROUTINE_SCHEMA         'u'
+#define PG_DIAG_TRIGGER_NAME           'g'
+#define PG_DIAG_TRIGGER_TABLE          'i'
+#define PG_DIAG_TRIGGER_SCHEMA         'h'

Not all appear to have a way of setting the value within the ereport
interface. For example, there is nothing like "errrelation_column()"
(or "errrelcol()", as I call it) to set PG_DIAG_ROUTINE_NAME. This is
something I haven't touched.

When I sent this patch first time, then one issue was new functions
for these fields. Tom proposal was using a generic function for these
new fields. These fields holds separated values, but in almost all
cases some combinations are used - "ROUTINE_NAME, ROUTINE_SCHEMA",
"TABLE_NAME, TABLE_SCHEMA" - so these fields are not independent -
this is difference from original ErrorData fields - so axillary
functions doesn't follow these fields - because it is not practical.

src/backend/utils/adt/domains.c
162: (errcode(ERRCODE_CHECK_VIOLATION),

these exceptions are related to domains - we has not adequate fields
now - and these fields are not in standards

it needs some like DOMAIN_NAME and DOMAIN_SCHEMA ???

Hmm. I'm not sure that it's worth it.

I took a look at recent JDBC documentation, because I'd expect it to
offer the most complete support for exposing this in exception
classes. Turns out that it does not expose things at as fine a
granularity as you have here at all, which is disappointing. It seems
to suppose that just a vendor code and "cause" will be sufficient. If
you're using this one proprietary database, there is a command that'll
let you get diagnostics. The wisdom for users of another proprietary
database seems to be that you just use stored procedures. So I agree
that CONSTRAINT NAME should be unset in the event of uncatalogued,
unnamed not null integrity constraint violations. The standard seems
to be loose on this, and I think we'll have to be too. However, I'd
find it pretty intolerable if we were inconsistent between ereport()
callsites that were *effectively the same error* - this could result
in a user's application breaking based on the phase of the moon.

Do you suppose it's worth stashing the last set of these fields to one
side, and exposing this through an SQL utility command, or even a
bundled function? I don't imagine that it's essential to have that
right away, but it's something to consider.

I understand, but fixing any ereport in core is difficult for
processing. So coverage only some subset is practical (first stage) -
with some basic infrastructure in core all other patches with better
covering will be simpler for review and for commit too. RI and
constraints is more often use cases where you would to parse error
messages - these will be covered in first stage.

Regards

Pavel

Show quoted text

--
Peter Geoghegan http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services