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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
