Patch review: make RAISE without arguments work like Oracle

I'd like to mark this as Ready for Committer :)

Review below.

Cheers,
David.

== Submission review ==

* Is the patch in context diff format?

Yes.

* Does it apply cleanly to the current CVS HEAD?

Yes, with a few offsets.

patch -p0 < ../reraise_issue_PG_v1.patch
patching file src/pl/plpgsql/src/pl_exec.c
Hunk #9 succeeded at 2427 (offset 2 lines).
Hunk #10 succeeded at 2663 (offset 2 lines).
patching file src/pl/plpgsql/src/plpgsql.h

* Does it include reasonable tests, necessary doc patches, etc?

No. Please find new patch attached with one test and one change
to the docs.

== Usability review ==

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

* Does the patch actually implement that?

Yes.

* Do we want that?

While the discussion was not long or extensive, the consensus
seemed to be yes.

* Do we already have it?

No.

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

Not applicable, as far as I understand the spec.

* Does it include pg_dump support (if applicable)?

Not applicable.

* Are there dangers?

Old code may depend on the previous behavior.

* Have all the bases been covered?

== Feature test ==

Apply the patch, compile it and test:

* Does the feature work as advertised?

Yes.

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

Not that I've found.

* Are there any assertion failures or crashes?

No.

**Review should be done with the ''configure'' options ''--enable-cassert'' and ''--enable-debug'' turned on; see [[Working with CVS]] for a full example. Those will help find issues with the code that might otherwise be missed. A copy of PostgreSQL built using these parameters will be substantially slower than one built without them. If you're working on something performance-related, such as testing whether a patch slows anything down, be sure to build without these flags before testing execution speed. You can turn off the assert testing, the larger of the slowdowns, at server start time by putting ''debug_assertions = false'' in your postgresql.conf. See [http://www.postgresql.org/docs/current/static/runtime-config-developer.html Developer Options] for more details about that setting; it defaults to true in builds done with --enable-cassert.

== Performance review ==

* Does the patch slow down simple tests?

Not that I've found, although anything involving exceptions in
PL/pgsql performs pretty poorly in stock PostgreSQL.

* If it claims to improve performance, does it?

It makes no such claim.

* Does it slow down other things?

Not that I've found.

== 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?

Not that I can see.

* Will it work on Windows/BSD etc?

No reason it shouldn't. Haven't tested those platforms.

* Are the comments sufficient and accurate?

Yes.

* Does it do what it says, correctly?

Yes, as far as I can tell.

* 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 that can cause problems?

Not that I've found, except as noted above re: apps based on the
previous behavior.

--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate