Re: Review of Writeable CTE Patch

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> wrote:

> Here's an updated patch. This includes the fix mentioned earlier, some
> comment improvements and making CopySnapshot() static again. I also
> changed all references to this feature to "DML WITH" for consistency.
> I'm not sure if we want to keep it, but it should now be easier to
> change in the future.

Hi, I'm reviewing the writable CTE patch. The code logic seems to be
pretty good, but I have a couple of comments about error cases:

* Did we have a consensus about user-visible "DML WITH" messages?
The term is used in error messages in many places, for example:
"DML WITH without RETURNING is only allowed inside an unreferenced CTE"
Since we don't use "DML WITH" nor "CTE" in documentation,
I'd like to avoid such technical acronyms in logs if we had better names,
or we should have a section to explain them in docs.

* What can I do to get "Recursive DML WITH statements are not supported"
message? I get syntax errors before I get the message because We don't
support UNIONs with RETURNING queries. Am I missing something?

=# UPDATE tbl SET i = i + 1 WHERE i = 1
UNION ALL
UPDATE tbl SET i = i + 1 WHERE i = 2;
ERROR: syntax error at or near "UNION"

* The patch includes regression tests, but no error cases in it.
More test cases are needed for stupid queries.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center