Re: errcontext support in PL/Perl - Mailing list pgsql-hackers

From the patch review group this evening, courtesy of Webb Sprague and
Brent Dombrowski.  I'm replying on their behalf so that this response
is in the correct thread.

Looks like Peter also reviewed, but I'm forwarding this on anyway.

(Note: Problem running regressions because make check doesn't descend
automatically into the plperl -- we built with --with-perl
successfully, but it doesn't change the behavior of make check.
However, the tests do work with patch.)

Per the wiki guidelines:

== Submission review ==

* Is the patch in the standard form?

Yes,

* Does it apply cleanly to the current CVS HEAD?

Yes

* Does it include reasonable tests, docs, etc?

Inline comments suffice.

== Usability review ==

Read what the patch is supposed to do, and consider:

* Does the patch actually implement that?

Yes, built in to the expected test output.

* Do we want that?

Yes, based on the patch to Python that does similar stuff.

* Do we already have it?

No

* Does it follow SQL spec, or the community-agreed behavior?

There was extensive discussion on the list about the desirability of
the context messages, and the community seems to agree in the utility

* Are there dangers?

No. (??)

* Have all the bases been covered?

We guess...

== Feature test ==

Apply the patch, compile it and test:

* Does the feature work as advertised?

Make check passes, and expected output is provided (problem with
automatically checking all)

* Are there corner cases the author has failed to consider?

Can't think of any -- popping off an empty stack or such worries me,
but I couldn't determine if there was a risk of this in the code.

== Performance review ==

* Does the patch slow down simple tests?

No.

* If it claims to improve performance, does it?

NA

* Does it slow down other things?

No (?)

== Coding review ==

Read the changes to the code in detail and consider:

* Does it follow the project
[http://developer.postgresql.org/pgdocs/postgres/source.html coding
guidelines]?

Yes

* Are there portability issues?

None that we can see

* Will it work on Windows/BSD etc?

No issues we can see

* Are the comments sufficient and accurate?

Yes

* Does it do what it says, correctly?

Yes

* Does it produce compiler warnings?

No

* Can you make it crash?

No

== Architecture review ==

Consider the changes to the code in the context of the project as a whole:

* Is everything done in a way that fits together coherently with other
features/modules?

Yes

* Are there interdependencies than can cause problems?

No, it is a simple set of changes all within plperl