Re: Writeable CTEs patch

On 2010-02-08 18:42 +0200, Robert Haas wrote:
> On Thu, Feb 4, 2010 at 11:57 AM, Marko Tiikkaja
>> Here's an updated patch. Only changes from the previous patch are
>> fixing the above issue and a regression test for it.
>
> - 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?

No objection to changing that.

> - 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.

Hmm. Right. That sounds like the right thing to do. Another option
(which I seem to recall we've discussed before) is to not allow any
SELECT statements between DML WITHs, but I think this is what we should
go for.

> - 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).

Ok, I'll look into that.

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

You're right. Will fix.

> - 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.

My bison-fu is not exactly strong, but I can look at the feasibility of
that.

> - 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.

I don't see how that would work. We'd still potentially have many
different types of DML operations to deal with and that wouldn't help at
all at distinguishing which operation actually caused the error. Or did
I misunderstand?

> 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.

"INSERT, UPDATE and DELETE" is quite long and "non-SELECT" is a bit
clumsy IMO. But I don't really have anything better to offer, either.

Regards,
Marko Tiikkaja