Re: patch: enhanced get diagnostics statement 2

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
>