Re: Writeable CTE patch

On Sun, Nov 15, 2009 at 14:27, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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