Re: Writeable CTE patch

On Sun, Nov 15, 2009 at 14:27, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> I wrote:
>>
>> Attached is the latest version of this patch.

Find attached a incremental diff with the following changes:
-get rid of operation argument to InitResultRelInfo, its unused now
-add some asserts to make sure places we use subplanstate now that it
can be null
(*note* AFAICT its a cant happen, but it made me nervous hence the Asserts)
-remove unneeded plannodes.h includes
-minor whitespace fix

Other comments:
You have an "XXX we should probably update the snapshot a bit
differently".  Any plans on that?
Thats quite a bit of new code in ExecutePlan, worth splitting into its
own function?

Also, after reading through the previous threads; it was not
immediately obvious that you dealt with
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00566.php by
only allowing selects or values at the top level of with.

Find below the standard review boilerplate from
http://wiki.postgresql.org/wiki/Reviewing_a_Patch

Summary: looks ready for a commiter to me after above comments are addressed.

Submission review:
 *Is the patch in context diff format?
  Yes
 * Does it apply cleanly to the current CVS HEAD?
  Yes, with fuzz
 * Does it include reasonable tests, necessary doc patches, etc?
  Yes

Usability review:
 Read what the patch is supposed to do, and consider:
 * Does the patch actually implement that?
  Yes
 * Do we want that?
  Yes
 * Do we already have it?
  No
 * Does it follow SQL spec, or the community-agreed behavior?
  Yes
 * Does it include pg_dump support (if applicable)?
  N/A
 * Are there dangers?
  No
 * Have all the bases been covered?
  All the ones I can see

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 could trigger
 * Are there any assertion failures or crashes?
  No
 o Review should be done with the configure options --enable-cassert
and --enable-debug turned on;
  Yes

Performance review:
 *Does the patch slow down simple tests:
  No
 *If it claims to improve performance, does it?
  N/A
 *Does it slow down other things
  No

Coding review:
 Read the changes to the code in detail and consider:
 * Does it follow the project coding guidelines?
  Yes
 * Are there portability issues?
  No
 * Will it work on Windows/BSD etc?
  Yes
 * 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?
  I think so.
 * Are there interdependencies than can cause problems?
  No