Re: Writeable CTEs patch - Mailing list pgsql-hackers

On Thu, Feb 4, 2010 at 11:57 AM, Marko Tiikkaja
<marko.tiikkaja@cs.helsinki.fi> wrote:
> On 2010-02-04 18:04 UTC+2, I wrote:
>> While working on the docs, I noticed one problem with the patch itself:
>> it doesn't handle multi-statement DO INSTEAD rules correctly.  I'm going
>> to submit a fix for that later.
>
> Here's an updated patch.  Only changes from the previous patch are
> fixing the above issue and a regression test for it.

The comments on the parts I asked about before are much better in this
version.  A few other things that I notice:

- I'm not sure that canSetTag is the right name for the additional
argument to ExecInsert/ExecUpdate/ExecDelete.  OTOH, I'm not sure it's
the wrong name either.  But should we use something like
isTopLevelQuery?

- It appears that we pull out all of the DML statements first and run
them in order, but I'm not sure that's the right thing to do.
Consider:

WITH x AS (INSERT ...), y AS (SELECT ...), z AS (INSERT ...) SELECT ...

I would assume we would do x, CCI, do y, do z, CCI, do main query, but
I don't think that's what this implements.  The user might be
surprised to find out that y sees the effects of z.

- I think that the comment in analyzeCTE that says /* Check that we
got something reasonable */ could be fleshed out a bit.  You could
still reference transformRangeSubselect, for example, but then explain
why the checks here are different (viz, CTEs can contain DML).

- The comment for RegisterSnapshotCopy identifies the function name as
RegisterSnapshot; I think this is a copy-and-pasteo.

- It seems like the gram.y changes for common_table_expr might benefit
from some factoring; that is, create a production (or find a suitable
existing one) for "statements of the sort that can appear within
CTEs", and then use that in common_table_expr.  Or maybe this doesn't
work; I haven't tried it.

- I still don't much like the idea of using DML WITH in error
messages.  One idea I had (which might suck, but I'm just throwing it
out there) is to change hasDmlWith to an integer bitmap with a bit for
each of insert, update, and delete.  But it may be better still to
just rephrase the error messages.  Could we just write, e.g.
"non-SELECT statements are not allowed within a cursor declaration?"
Or we could say "INSERT, UPDATE, and DELETE statements are not allowed
within a cursor declaration", but I'm thinking we may want to allow
things like COPY and EXPLAIN inside CTEs in the future, too, and
they'll presumably be treated similarly to DML.

For the record, Tom or whoever should feel to swoop in here at any
time, or add to any of this.  I'm just making suggestions until the
big guns show up.

...Robert