Skip site navigation (1) Skip section navigation (2)

Re: Review of Writeable CTE Patch

From: Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>, Merlin Moncure <mmoncure(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Review of Writeable CTE Patch
Date: 2010-02-03 16:18:17
Message-ID: 4B69A1C9.4090702@cs.helsinki.fi (view raw or flat)
Thread:
Lists: pgsql-hackers
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

In response to

Responses

pgsql-hackers by date

Next:From: Simon RiggsDate: 2010-02-03 16:21:26
Subject: Re: [COMMITTERS] pgsql: Assorted cleanups in preparation for using a map file to support
Previous:From: Tom LaneDate: 2010-02-03 16:13:47
Subject: Re: Review of Writeable CTE Patch

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group