Re: patch: enhanced get diagnostics statement 2

(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