On 2010-02-03 16:53 UTC+2, Robert Haas wrote: > Some thoughts: > > The comments in standard_ExecutorStart() don't do a good job of > explaining WHY the code does what it does - they just recapitulate > what you can already see from reading the code. You say "If there are > DML WITH statements, we always need to use the CID and copy the > snapshot." That's self-evident from the following code. What's not > clear is why this is necessary, and the comment doesn't make any > attempt to explain it. The second half of the if statement has the > same problem.
Ok, I'll try to make this more clear. > In ExecTopLevelStmtIsReadOnly, you might perhaps want to rephase the > comment in a way that doesn't use the word "Ehm." Like maybe: "Even > if this function returns true, the statement might still contain > INSERT, > UPDATE, or DELETE statements within a CTE; we only check the top-level > statement." Also, there should be a newline immediately before the > function name, per our usual style conventions. That comment tries to emphasize the fact that I can't think of any reasonable name for that particular function. If the name looks OK, I can update the comment. > The comment in analyzeCTE that says "Many of these conditions are > impossible given restrictions of the grammar, but check 'em anyway." > makes less sense with this patch than it did formerly and may need to > be rethought... and I'm not sure there's any reason to change this > elog() an Assert. Ok, I'll look at this. > In both analyzeCTE() and checkWellFormedRecursion(), I don't like just > removing the assertions there; we should try to assert something a bit > more sensible, like maybe !IsA(cte->ctequery, Query). This patch > removes a number of other assertions as well, but I don't know enough > about those other spots to judge whether all of those cases are > sensible. I'll look through these again. > The only change to parse_relation.c is the addition of a #include; is > this needed? No, I thought I had removed that long time ago. Will remove. > The documentation changes for INSERT, UPDATE, and DELETE seem > inadequate, because they add a reference to with_query with no > corresponding explanation of what a with_query might be. Ok, I'll add this. > The limitations of INSERT/UPDATE/DELETE-within-WITH should be > documented somewhere: top level CTE only, and no DO ALSO or > conditional DO INSTEAD rules. If we don't intend to remove this > limitation in a future release, we should probably also document that. > I believe there are some other caveats that we've discussed before, > too, though I'm not sure if they're still true. Stuff like: > > - CTEs will be executed to completion in sequential order before the > main statement begins execution > - each CTE will see the results of CTEs already executed, and the main > statement will see the results of all CTEs > - but queries within each CTE still won't see their own updates (a > reference to whatever section of the manual we talk about this in > would probably be good) > - possible pitfalls of CTEs not being pipelined Right. The documentation in its current state is definitely lacking. I've tried to focus all the time I have in making this patch technically good. 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
