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
describedin 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
againstthe 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
myselfto make some revisions, including moving the list of fields into a list. I've attached a new patch with these
changesbelow.
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,
giventhe 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
getthis 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
