Thread

1. patch: enhanced get diagnostics statement 2

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-06-02T08:39:05Z

   Hello
   
   This patch enhances a GET DIAGNOSTICS statement functionality. It adds
   a possibility of access to exception's data. These data are stored on
   stack when exception's handler is activated - and these data are
   access-able everywhere inside handler. It has a different behave (the
   content is immutable inside handler) and therefore it has modified
   syntax (use keyword STACKED). This implementation is in conformance
   with ANSI SQL and SQL/PSM  - implemented two standard fields -
   RETURNED_SQLSTATE and MESSAGE_TEXT and three PostgreSQL specific
   fields - PG_EXCEPTION_DETAIL, PG_EXCEPTION_HINT and
   PG_EXCEPTION_CONTEXT.
   
   The GET STACKED DIAGNOSTICS statement is allowed only inside
   exception's handler. When it is used outside handler, then diagnostics
   exception 0Z002 is raised.
   
   This patch has no impact on performance. It is just interface to
   existing stacked 'edata' structure. This patch doesn't change a
   current behave of GET DIAGNOSTICS statement.
   
   CREATE OR REPLACE FUNCTION public.stacked_diagnostics_test02()
    RETURNS void
    LANGUAGE plpgsql
   AS $function$
   declare _detail text; _hint text; _message text;
   begin
     perform ...
   exception when others then
     get stacked diagnostics
           _message = message_text,
           _detail = pg_exception_detail,
           _hint = pg_exception_hint;
     raise notice 'message: %, detail: %, hint: %', _message, _detail, _hint;
   end;
   $function$
   
   All regress tests was passed.
   
   Regards
   
   Pavel Stehule
   

2. Re: patch: enhanced get diagnostics statement 2

   Shigeru Hanada <shigeru.hanada@gmail.com> — 2011-07-06T09:31:37Z

   (2011/06/02 17:39), Pavel Stehule wrote:
   > This patch enhances a GET DIAGNOSTICS statement functionality. It adds
   > a possibility of access to exception's data. These data are stored on
   > stack when exception's handler is activated - and these data are
   > access-able everywhere inside handler. It has a different behave (the
   > content is immutable inside handler) and therefore it has modified
   > syntax (use keyword STACKED). This implementation is in conformance
   > with ANSI SQL and SQL/PSM  - implemented two standard fields -
   > RETURNED_SQLSTATE and MESSAGE_TEXT and three PostgreSQL specific
   > fields - PG_EXCEPTION_DETAIL, PG_EXCEPTION_HINT and
   > PG_EXCEPTION_CONTEXT.
   > 
   > The GET STACKED DIAGNOSTICS statement is allowed only inside
   > exception's handler. When it is used outside handler, then diagnostics
   > exception 0Z002 is raised.
   > 
   > This patch has no impact on performance. It is just interface to
   > existing stacked 'edata' structure. This patch doesn't change a
   > current behave of GET DIAGNOSTICS statement.
   > 
   > CREATE OR REPLACE FUNCTION public.stacked_diagnostics_test02()
   >   RETURNS void
   >   LANGUAGE plpgsql
   > AS $function$
   > declare _detail text; _hint text; _message text;
   > begin
   >    perform ...
   > exception when others then
   >    get stacked diagnostics
   >          _message = message_text,
   >          _detail = pg_exception_detail,
   >          _hint = pg_exception_hint;
   >    raise notice 'message: %, detail: %, hint: %', _message, _detail, _hint;
   > end;
   > $function$
   > 
   > All regress tests was passed.
   
   Hi Pavel,
   
   I've reviewed your patch according to the page "Reviewing a patch".
   During the review, I referred to Working-Draft of SQL 2003 to confirm
   the SQL specs.
   
   Submission review
   =================
   * The patch is in context diff format.
   * The patch couldn't be applied cleanly to the current head.  But it
   requires only one hunk to be offset, and it could be fixed easily.
   I noticed that new variables needs_xxx, which were added to struct
   PLpgSQL_condition, are not used at all.  They should be removed, or
   something might be overlooked.
   * The patch includes reasonable regression tests.  The patch also
   includes hunks for pl/pgsql document which describes new
   feature.  But it would need some corrections:
     - folding too-long lines
     - fixing some grammatical errors (maybe)
     - clarify difference between CURRENT and STACKED
   I think that adding new section for GET STACKED DIAGNOSTICS would help
   to clarify the difference, because the keyword STACKED can be used only
   in exception clause, and available information is different from the one
   available for GET CURRENT DIAGNOSTICS.  Please find attached a patch
   which includes a proposal for document though it still needs review by
   English speaker.
   
   Usability review
   ================
   * The patch extends GET DIAGNOSTICS syntax to accept new keywords
   CURRENT and STACKED, which are described in the SQL/PSM standard.  This
   feature allows us to retrieve exception information in EXCEPTION clause.
   Naming of PG-specific fields might be debatable.
   * I think it's useful to get detailed information inside EXCEPTION clause.
   * We don't have this feature yet.
   * This patch follows SQL spec of GET DIAGNOSTICS, and extends about
   PG-specific variables.
   * pg_dump support is not required for this feature.
   * AFAICS, this patch doesn't have any danger, such as breakage of
   backward compatibility.
   
   Feature test
   ============
   * The new feature introduced by the patch works well.
   I tested about:
     - CURRENT doesn't affect existing feature
     - STACKED couldn't be used outside EXCEPTION clause
     - Values could be retrieved via RETURNED_SQLSTATE, MESSAGE_TEXT,
       PG_EXCEPTION_DETAIL, PG_EXCEPTION_HINT and PG_EXCEPTION_CONTEXT
     - Invalid item names properly cause error.
   * I'm not so familiar to pl/pgsql, but ISTM that enough cases are
   considered about newly added diagnostics items.
   * I didn't see any crash during my tests.
   
   In conclusion, this patch still needs some effort to be "Ready for
   Committer", so I'll push it back to "Waiting on Author".
   
   Regards,
   -- 
   Shigeru Hanada
   

3. Re: patch: enhanced get diagnostics statement 2

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-07-07T07:30:55Z

   Hello
   
   thank you very much for review.
   
   I cleaned patch and merged your documentation patch
   
   I hope, this is all - a language correction should do some native speaker
   
   Regards
   
   Pavel Stehule
   
   2011/7/6 Shigeru Hanada <shigeru.hanada@gmail.com>:
   > (2011/06/02 17:39), Pavel Stehule wrote:
   >> This patch enhances a GET DIAGNOSTICS statement functionality. It adds
   >> a possibility of access to exception's data. These data are stored on
   >> stack when exception's handler is activated - and these data are
   >> access-able everywhere inside handler. It has a different behave (the
   >> content is immutable inside handler) and therefore it has modified
   >> syntax (use keyword STACKED). This implementation is in conformance
   >> with ANSI SQL and SQL/PSM  - implemented two standard fields -
   >> RETURNED_SQLSTATE and MESSAGE_TEXT and three PostgreSQL specific
   >> fields - PG_EXCEPTION_DETAIL, PG_EXCEPTION_HINT and
   >> PG_EXCEPTION_CONTEXT.
   >>
   >> The GET STACKED DIAGNOSTICS statement is allowed only inside
   >> exception's handler. When it is used outside handler, then diagnostics
   >> exception 0Z002 is raised.
   >>
   >> This patch has no impact on performance. It is just interface to
   >> existing stacked 'edata' structure. This patch doesn't change a
   >> current behave of GET DIAGNOSTICS statement.
   >>
   >> CREATE OR REPLACE FUNCTION public.stacked_diagnostics_test02()
   >>   RETURNS void
   >>   LANGUAGE plpgsql
   >> AS $function$
   >> declare _detail text; _hint text; _message text;
   >> begin
   >>    perform ...
   >> exception when others then
   >>    get stacked diagnostics
   >>          _message = message_text,
   >>          _detail = pg_exception_detail,
   >>          _hint = pg_exception_hint;
   >>    raise notice 'message: %, detail: %, hint: %', _message, _detail, _hint;
   >> end;
   >> $function$
   >>
   >> All regress tests was passed.
   >
   > Hi Pavel,
   >
   > I've reviewed your patch according to the page "Reviewing a patch".
   > During the review, I referred to Working-Draft of SQL 2003 to confirm
   > the SQL specs.
   >
   > Submission review
   > =================
   > * The patch is in context diff format.
   > * The patch couldn't be applied cleanly to the current head.  But it
   > requires only one hunk to be offset, and it could be fixed easily.
   > I noticed that new variables needs_xxx, which were added to struct
   > PLpgSQL_condition, are not used at all.  They should be removed, or
   > something might be overlooked.
   > * The patch includes reasonable regression tests.  The patch also
   > includes hunks for pl/pgsql document which describes new
   > feature.  But it would need some corrections:
   >  - folding too-long lines
   >  - fixing some grammatical errors (maybe)
   >  - clarify difference between CURRENT and STACKED
   > I think that adding new section for GET STACKED DIAGNOSTICS would help
   > to clarify the difference, because the keyword STACKED can be used only
   > in exception clause, and available information is different from the one
   > available for GET CURRENT DIAGNOSTICS.  Please find attached a patch
   > which includes a proposal for document though it still needs review by
   > English speaker.
   >
   > Usability review
   > ================
   > * The patch extends GET DIAGNOSTICS syntax to accept new keywords
   > CURRENT and STACKED, which are described in the SQL/PSM standard.  This
   > feature allows us to retrieve exception information in EXCEPTION clause.
   > Naming of PG-specific fields might be debatable.
   > * I think it's useful to get detailed information inside EXCEPTION clause.
   > * We don't have this feature yet.
   > * This patch follows SQL spec of GET DIAGNOSTICS, and extends about
   > PG-specific variables.
   > * pg_dump support is not required for this feature.
   > * AFAICS, this patch doesn't have any danger, such as breakage of
   > backward compatibility.
   >
   > Feature test
   > ============
   > * The new feature introduced by the patch works well.
   > I tested about:
   >  - CURRENT doesn't affect existing feature
   >  - STACKED couldn't be used outside EXCEPTION clause
   >  - Values could be retrieved via RETURNED_SQLSTATE, MESSAGE_TEXT,
   >    PG_EXCEPTION_DETAIL, PG_EXCEPTION_HINT and PG_EXCEPTION_CONTEXT
   >  - Invalid item names properly cause error.
   > * I'm not so familiar to pl/pgsql, but ISTM that enough cases are
   > considered about newly added diagnostics items.
   > * I didn't see any crash during my tests.
   >
   > In conclusion, this patch still needs some effort to be "Ready for
   > Committer", so I'll push it back to "Waiting on Author".
   >
   > Regards,
   > --
   > Shigeru Hanada
   >
   

4. Re: patch: enhanced get diagnostics statement 2

   David E. Wheeler <theory@kineticode.com> — 2011-07-10T06:01:57Z

   On Jul 7, 2011, at 12:30 AM, Pavel Stehule wrote:
   
   > thank you very much for review.
   
   I thank you, too, Hanada-san. I was assigned to review this patch, but you beat me to it. So now I'll do the follow-up review.
   
   > I cleaned patch and merged your documentation patch
   > 
   > I hope, this is all - a language correction should do some native speaker
   
   Contents & Purpose
   ==================
   The patch extends the `GET DIAGNOSTICS` syntax to accept new two new keywords,  `CURRENT` and `STACKED`, which are described in the SQL/PSM standard. This feature allows one to retrieve exception information in an `EXCEPTION` block.
   
   The patch also adds three PostgreSQL-specific fields:
   
   * `PG_EXCEPTION_DETAIL` contains the exception detail message
   * `PG_EXCEPTION_HINT` contains the exception hint, if any
   * `PG_EXCEPTION_CONTEXT` contains lines that describes call stack
   
   Submission Review
   =================
   The patch is a unified diff, and applies cleanly to master at 89fd72cb. The regression tests all pass successfully against the new patch, so the test cases are sane and do cover the new behavior.
   
   The patch includes regression tests which appear to adequately cover the proposed functionality. They also contain documentation, although the wording, while understandable, needs the attention of a native speaker. I've taken it upon myself to make some revisions, including moving the list of fields into a list. I've attached a new patch with these changes below.
   
   Usability review
   ================
   * I agree that it's useful to get detailed information inside EXCEPTION clause.
   * We don't have this feature yet.
   * This patch follows SQL spec of GET DIAGNOSTICS, and extends about PG-specific variables.
   * pg_dump support is not required for this feature.
   * AFAICS, this patch doesn't have any danger, such as breakage of backward compatibility, thanks to the new `STACKED` keyword. I suppose there could be a conflict if someone had a variable named STACKED in the function, but I doubt it, given the context in which it's used.
   
   Feature test
   ============
   * The new feature introduced by the patch works well. I ran some basic tests and it worked very nicely. I'm excited to get this functionality!
   
   Conclusion
   ==========
   Attached is a new patch with my documentation changes (and in context diff format). The code looks clean and unobtrusive, the functionality it adds is useful, and overall I'd say it's ready for committer.
   
   Best,
   
   David
   
   

5. Re: patch: enhanced get diagnostics statement 2

   Alvaro Herrera <alvherre@commandprompt.com> — 2011-07-14T20:11:17Z

   A couple items for this patch:
   
   The docs state that the variable to receive each diagnostic value needs
   to be "of the right data type" but fails to specify what it is.  I think
   it'd be good to turn that <itemizedlist> into a table with three
   columns: name, type, description.
   
   This seems poor style:
   
   +                               case PLPGSQL_GETDIAG_ERROR_CONTEXT:
   +                               case PLPGSQL_GETDIAG_ERROR_DETAIL:
   +                               case PLPGSQL_GETDIAG_ERROR_HINT:
   +                               case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
   +                               case PLPGSQL_GETDIAG_MESSAGE_TEXT:
   +                                   if (!$2)
   +                                       ereport(ERROR,
   +                                           (errcode(ERRCODE_SYNTAX_ERROR),
   +                                            errmsg("EXCEPTION_CONTEXT or EXCEPTION_DETAIL or EXCEPTION_HINT or RETURNED_SQLSTATE or MESSAGE_TEXT are not allowed in current diagnostics statement"),
   +                                                    parser_errposition(@1)));
   +                                   
   
   
   I think we could replace this with something like
   
   +                                   if (!$2)
   +                                       ereport(ERROR,
   +                                           (errcode(ERRCODE_SYNTAX_ERROR),
   +                                            errmsg("diagnostic value %s is not allowed in GET CURRENT DIAGNOSTICS statement", stringify(ditem->kind)),
   
   
   Other than that, and a few grammar fixes in code comments, this seems
   good to me.
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

6. Re: patch: enhanced get diagnostics statement 2

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-07-14T20:25:56Z

   2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
   > A couple items for this patch:
   >
   > The docs state that the variable to receive each diagnostic value needs
   > to be "of the right data type" but fails to specify what it is.  I think
   > it'd be good to turn that <itemizedlist> into a table with three
   > columns: name, type, description.
   >
   > This seems poor style:
   >
   > +                               case PLPGSQL_GETDIAG_ERROR_CONTEXT:
   > +                               case PLPGSQL_GETDIAG_ERROR_DETAIL:
   > +                               case PLPGSQL_GETDIAG_ERROR_HINT:
   > +                               case PLPGSQL_GETDIAG_RETURNED_SQLSTATE:
   > +                               case PLPGSQL_GETDIAG_MESSAGE_TEXT:
   > +                                   if (!$2)
   > +                                       ereport(ERROR,
   > +                                           (errcode(ERRCODE_SYNTAX_ERROR),
   > +                                            errmsg("EXCEPTION_CONTEXT or EXCEPTION_DETAIL or EXCEPTION_HINT or RETURNED_SQLSTATE or MESSAGE_TEXT are not allowed in current diagnostics statement"),
   > +                                                    parser_errposition(@1)));
   > +
   >
   >
   > I think we could replace this with something like
   >
   > +                                   if (!$2)
   > +                                       ereport(ERROR,
   > +                                           (errcode(ERRCODE_SYNTAX_ERROR),
   > +                                            errmsg("diagnostic value %s is not allowed in GET CURRENT DIAGNOSTICS statement", stringify(ditem->kind)),
   >
   >
   > Other than that, and a few grammar fixes in code comments, this seems
   > good to me.
   >
   
   it is good idea
   
   Regards
   
   Pavel
   
   > --
   > Álvaro Herrera <alvherre@commandprompt.com>
   > The PostgreSQL Company - Command Prompt, Inc.
   > PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   >
   

7. Re: patch: enhanced get diagnostics statement 2

   Alvaro Herrera <alvherre@commandprompt.com> — 2011-07-14T20:35:02Z

   Excerpts from Pavel Stehule's message of jue jul 14 16:25:56 -0400 2011:
   > 2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
   > > A couple items for this patch:
   
   > it is good idea
   
   Thanks ... I expect you're going to resubmit the patch based on David's
   last version and my comments?
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   
   

8. Re: patch: enhanced get diagnostics statement 2

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-07-14T20:47:32Z

   2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
   > Excerpts from Pavel Stehule's message of jue jul 14 16:25:56 -0400 2011:
   >> 2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
   >> > A couple items for this patch:
   >
   >> it is good idea
   >
   > Thanks ... I expect you're going to resubmit the patch based on David's
   > last version and my comments?
   >
   
   yes, but tomorrow, time to go sleep
   
   Regards
   
   Pavel
   
   > --
   > Álvaro Herrera <alvherre@commandprompt.com>
   > The PostgreSQL Company - Command Prompt, Inc.
   > PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   >
   
   

9. Re: patch: enhanced get diagnostics statement 2

   Pavel Stehule <pavel.stehule@gmail.com> — 2011-07-15T07:33:53Z

   Hello
   
   2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
   > Excerpts from Pavel Stehule's message of jue jul 14 16:25:56 -0400 2011:
   >> 2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
   >> > A couple items for this patch:
   >
   >> it is good idea
   >
   > Thanks ... I expect you're going to resubmit the patch based on David's
   > last version and my comments?
   >
   
   yes, see a attachment
   
   Regards
   
   Pavel
   
   
   > --
   > Álvaro Herrera <alvherre@commandprompt.com>
   > The PostgreSQL Company - Command Prompt, Inc.
   > PostgreSQL Replication, Consulting, Custom Development, 24x7 support
   >
   

10. Re: patch: enhanced get diagnostics statement 2

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-07-18T18:49:11Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
    > 2011/7/14 Alvaro Herrera <alvherre@commandprompt.com>:
    >> Thanks ... I expect you're going to resubmit the patch based on David's
    >> last version and my comments?
    
    > yes, see a attachment
    
    Applied with some editorial adjustments.
    
    			regards, tom lane