Skip site navigation (1) Skip section navigation (2)

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
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackerspgsql-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:


pgsql-hackers by date

Next:From: Peter EisentrautDate: 2010-07-19 21:04:04
Subject: Re: standard_conforming_strings
Previous:From: Bruce MomjianDate: 2010-07-19 19:08:09
Subject: Re: TODO 9.0 done items removed

pgsql-rrreviewers by date

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

Privacy Policy | About PostgreSQL
Copyright © 1996-2018 The PostgreSQL Global Development Group