Review of Writeable CTE Patch - Mailing list pgsql-hackers

Marko,

Submission Review
-----------------
*) patch applies clean to HEADwever

*) applied tests ran ok

*) there is some documentation adjustments in the patch. possibly a
little underweight, but sufficient.

*) A couple of very minor things aside, I think this should be
accepted and passed for 8.5  although it could use another set of eyes
from someone more comfortable with this part of the code.

Usability Review
----------------
*) Works as advertised...'feels right'.  Found only one small issue
which Marko was already aware of and had adjusted for.

*) No, we don't already have it.  This is maybe the #2 most requested
feature, after in-core replication.

*) Yes it follows community-agreed behavior.

Feature Test
------------
*) No build issues.

*) The feature appears to work. Aside from the supplied tests, I
tested with much larger tables (million records) and tables with
triggers on them, sometimes in wacky combination:

with f as (delete from foo returning *) insert into foo select * from f;
with f as (insert into foo returning *) insert into foo select * from f;

This threw an assertion failure:
with recursive t as (insert into foo select * from t) values(true);

TRAP: FailedAssertion("!(((((Node*)(stmt))->type) == T_SelectStmt))",
File: "parse_cte.c", Line: 606)

Marko was already aware of it and has a fix ready.

*) I tested most of the error conditions and got them to fire.  No
unpleasant surprises.  The cursor error ("Non-SELECT cursors are not
implemented") is a little misleading.  Perhaps it should read
something like "Writeable CTE are not supported in cusor declaration"


Performance Review
------------------

*) Everything ran exactly as it should.  Huge updates rewritten as
writeable CTE delete + insert are slightly slower than raw insert but
this is expected.


Coding Review
-------------

*) A lot of the code is sgml/test/grammar changes that should be
relatively uncontroversial.  This is a small patch for what it does.

*) Should canSetTag be passed as the first agument to (for example) in
ExecInsert?  Is this the proper way to be passing this information?

*) execMain.c Line 1224
There is a blank code comment here.  IMO this section needs to be
documented better: the
CommandCounterIncrement is a critical part of how this works.

*) execMain.c Line 2033
The adjusted comment is confusing and seems to contradict itself.  I
would have wriiten: initialize them if they are not run in
EvalPlanQual().  Aside: is this an optimization?

*) CopySnapshot was promoted from static.  Is this legal/good idea?
Is a wrapper more appropriate?

Architecture Review
-------------------

*) Nothing jumped out at me.   The changes are small, so it really
comes down to if they were done properly/right spot.

Review Review
-------------

merlin