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

Attachment: stacked_diagnostics_3.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Reply via email to