Re: enhanced error fields

2012/7/2 Peter Geoghegan <peter(at)2ndquadrant(dot)com>:
> On 2 July 2012 15:19, Peter Geoghegan <peter(at)2ndquadrant(dot)com> wrote:
>> 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.
>
> 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(at)peterlaptop eelog]$ ls
> exceptions.sql  test_eelog_outer.sql
>
> Patch (eelog-plpgsql):
>
> [peter(at)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(at)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

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