Re: patch: enhanced get diagnostics statement 2

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

(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

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(dot)hanada(at)gmail(dot)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
>

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

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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Excerpts from Pavel Stehule's message of jue jul 14 16:25:56 -0400 2011:
> 2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of jue jul 14 16:25:56 -0400 2011:
>> 2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Hello

2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of jue jul 14 16:25:56 -0400 2011:
>> 2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)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(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2011/7/14 Alvaro Herrera <alvherre(at)commandprompt(dot)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