Re: Writeable CTE patch

Alex Hunsaker wrote:
> Find attached a incremental diff with the following changes:
> -get rid of operation argument to InitResultRelInfo, its unused now

Missed that one. Thanks.

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

Indeed, it shouldn't happen, but this seems like a decent precaution.

> Other comments:
> You have an "XXX we should probably update the snapshot a bit
> differently". Any plans on that?

I've looked into that, but couldn't find a better way. Maybe I should
take out my scuba gear for a new dive into the snapshot code..

> Thats quite a bit of new code in ExecutePlan, worth splitting into its
> own function?

Could probably be.

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

This is actually just something missing from the current implementation.
The relevant posts are in the same thread:
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00558.php and
the two follow-ups. The comment in ExecutePlan() is a bit misleading.
What I meant is that we don't call GetCurrentCommandId() in
standard_ExecutorStart(). Instead we get a new CID for every CTE with
INSERT/UPDATE/DELETE. That comment tried to point out the fact that
this strategy could fail if there was a non-SELECT query as the
top-level statement because we wouldn't increment the CID after the last
CTE. I did it this way because it works well for the purposes of this
patch and I didn't see an obvious way to determine whether we need a new
CID for the top-level statement or not.

I'll send an updated patch in a couple of days.

Regards,
Marko Tiikkaja