Patch review: make RAISE without arguments work like Oracle

From: David Fetter <david(at)fetter(dot)org>
To: Round Robin Reviewers <pgsql-rrreviewers(at)postgresql(dot)org>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Patch review: make RAISE without arguments work like Oracle
Date: 2010-07-19 20:18:45
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers pgsql-rrreviewers

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

Review below.


== Submission review ==

* Is the patch in context diff format?


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


* Do we want that?

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

* Do we already have it?


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


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

Not that I've found.

* Are there any assertion failures or crashes?


**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 [ 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 [ coding guidelines]?


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


* Does it do what it says, correctly?

Yes, as far as I can tell.

* Does it produce compiler warnings?


* Can you make it crash?


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


* 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>
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://

Remember to vote!
Consider donating to Postgres:

Attachment Content-Type Size
reraise_issue_PG_v2.patch text/plain 6.8 KB


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Eisentraut 2010-07-19 21:04:04 Re: standard_conforming_strings
Previous Message Bruce Momjian 2010-07-19 19:08:09 Re: TODO 9.0 done items removed

Browse pgsql-rrreviewers by date

  From Date Subject
Next Message Kevin Grittner 2010-07-20 22:29:14 CF process
Previous Message David Fetter 2010-07-19 19:02:56 patch pick: make RAISE without arguments work like Oracle