From: | Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> |
---|---|
To: | Shigeru Hanada <shigeru(dot)hanada(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: patch: enhanced get diagnostics statement 2 |
Date: | 2011-07-07 07:30:55 |
Message-ID: | CAFj8pRAsTgBzHSwLGgSzQOUi1kbWsbrswRSs_HYWSNDVAfv0MQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
>
Attachment | Content-Type | Size |
---|---|---|
getdiag-2.diff | text/x-patch | 20.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Florian Pflug | 2011-07-07 09:54:43 | Re: spinlock contention |
Previous Message | Ashutosh Bapat | 2011-07-07 07:05:37 | dropping table in testcase alter_table.sql |