Re: Review of Writeable CTE Patch
От | Robert Haas |
---|---|
Тема | Re: Review of Writeable CTE Patch |
Дата | |
Msg-id | 603c8f071002030653h7c1380d2qcec6eb6454fa3b7d@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: Review of Writeable CTE Patch (Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi>) |
Ответы |
Re: Review of Writeable CTE Patch
(Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi>)
Re: Review of Writeable CTE Patch (Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi>) |
Список | pgsql-hackers |
On Wed, Feb 3, 2010 at 5:31 AM, Marko Tiikkaja <marko.tiikkaja@cs.helsinki.fi> wrote: > On 2010-02-03 11:04, Takahiro Itagaki wrote: >> * The patch includes regression tests, but no error cases in it. >> More test cases are needed for stupid queries. > > Here's an updated patch. 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. 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. InitPlan makes some references to "leader" scan states, but there's no explanation of what exactly those are. 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. 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. The only change to parse_relation.c is the addition of a #include; is this needed? 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. 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 discussedbefore, 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 ...Robert
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Alex HunsakerДата:
Сообщение: Re: Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]
Следующее
От: Robert HaasДата:
Сообщение: Re: Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]