Prev: ECPGset_var
Next: [HACKERS] unfathomable comment in psqlscan.l
From: Marko Tiikkaja on 3 Feb 2010 04:09
Hi,

On 2010-02-03 11:04 UTC+2, Takahiro Itagaki wrote:
> 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.

We have yet to reach a consensus on the name for this feature. I don't
think we have any really good candidates, but I like "DML WITH" best so far.

> * 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"

WITH RECURSIVE t AS (INSERT INTO foo SELECT * FROM t) VALUES(true);
would do that. You can do the same with UPDATE .. FROM and DELETE .. USING.

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

Ok, I'll add these and send an updated patch in a few hours.


Regards,
Marko Tiikkaja

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 3 Feb 2010 09:53
On Wed, Feb 3, 2010 at 5:31 AM, Marko Tiikkaja
<marko.tiikkaja(a)cs.helsinki.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Tom Lane on 3 Feb 2010 10:58
Robert Haas <robertmhaas(a)gmail.com> writes:
> On Wed, Feb 3, 2010 at 4:09 AM, Marko Tiikkaja
> <marko.tiikkaja(a)cs.helsinki.fi> wrote:
>> We have yet to reach a consensus on the name for this feature. �I don't
>> think we have any really good candidates, but I like "DML WITH" best so far.

> Why can't we complain about the actual SQL statement the user issued?
> Like, say:
> INSERT requires RETURNING when used within a referenced CTE

We could probably make that work for error messages, but what about
documentation? It's going to be awkward to write something like
"INSERT/UPDATE/DELETE RETURNING" every time we need to make a general
statement about the behavior of all three.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Marko Tiikkaja on 3 Feb 2010 11:04
Hi,

On 2010-02-03 16:09 UTC+2, Robert Haas wrote:
> Why can't we complain about the actual SQL statement the user issued?
> Like, say:
>
> INSERT requires RETURNING when used within a referenced CTE

The SELECT equivalent of this query looks like this:
=> with recursive t as (select * from t) values(true);
ERROR: recursive query "t" does not have the form non-recursive-term
UNION [ALL] recursive-term

but I didn't want to throw people off to think that they can use
INSERT/UPDATE/RETURNING in a RECURSIVE CTE, just to get complaints about
syntax error.


Regards,
Marko Tiikkaja


--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Robert Haas on 3 Feb 2010 11:08
On Wed, Feb 3, 2010 at 10:58 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas(a)gmail.com> writes:
>> On Wed, Feb 3, 2010 at 4:09 AM, Marko Tiikkaja
>> <marko.tiikkaja(a)cs.helsinki.fi> wrote:
>>> We have yet to reach a consensus on the name for this feature.  I don't
>>> think we have any really good candidates, but I like "DML WITH" best so far.
>
>> Why can't we complain about the actual SQL statement the user issued?
>> Like, say:
>> INSERT requires RETURNING when used within a referenced CTE
>
> We could probably make that work for error messages, but what about
> documentation?  It's going to be awkward to write something like
> "INSERT/UPDATE/DELETE RETURNING" every time we need to make a general
> statement about the behavior of all three.

The current patch includes a total of 5 lines of text documenting this
new feature (plus one example), so the issue doesn't really arise.

If, as I believe, more documentation is needed, then we may need to
think about how to handle this, but it's hard to speculate without a
bit more context.

....Robert

--
Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

First  |  Prev  |  Next  |  Last
Pages: 1 2 3 4
Prev: ECPGset_var
Next: [HACKERS] unfathomable comment in psqlscan.l