Re: enhanced error fields - Mailing list pgsql-hackers

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