Re: patch: enhanced get diagnostics statement 2

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