Re: Review of Writeable CTE Patch

On Wed, Feb 3, 2010 at 5:31 AM, Marko Tiikkaja
<marko(dot)tiikkaja(at)cs(dot)helsinki(dot)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 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

...Robert