Re: Review of Writeable CTE Patch

Hi,

Here's an updated patch. This includes the fix mentioned earlier, some
comment improvements and making CopySnapshot() static again. I also
changed all references to this feature to "DML WITH" for consistency.
I'm not sure if we want to keep it, but it should now be easier to
change in the future.

On 2010-01-26 16:13, Merlin Moncure wrote:
> Marko was already aware of it and has a fix ready.

This one includes it.

> *) 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"

Ok, changed this one.

> *) 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.

Added some more comments.

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

I tried, but I can't think of a better wording for that comment :-(
This is not really an optimization, just doing the right thing. The
performance difference here is negligible.

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

Added new function RegisterSnapshotCopy() per Alvaro's suggestion.

Thanks a lot for reviewing!

Regards,
Marko Tiikkaja