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
[[Workingwith CVS]] for a full example.  Those will help find issues with the code that might otherwise be missed.  A
copyof PostgreSQL built using these parameters will be substantially slower than one built without them.  If you're
workingon something performance-related, such as testing whether a patch slows anything down, be sure to build without
theseflags before testing execution speed.  You can turn off the assert testing, the larger of the slowdowns, at server
starttime by putting ''debug_assertions = false'' in your postgresql.conf.  See
[http://www.postgresql.org/docs/current/static/runtime-config-developer.htmlDeveloper Options] for more details about
thatsetting; 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@fetter.org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david.fetter@gmail.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