Review of Writeable CTE Patch
in [PgSql]
|
From: Marko Tiikkaja on 3 Feb 2010 11:18 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 -- 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 11:13 Robert Haas <robertmhaas(a)gmail.com> writes: > On Wed, Feb 3, 2010 at 10:58 AM, Tom Lane <tgl(a)sss.pgh.pa.us> wrote: >> 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. Well, that's certainly not going to be nearly sufficient. I think what you meant is "Marko hasn't bothered with documentation". There is going to need to be discussion in the RULES chapter, in the page describing returned command tags, and probably six other places that aren't coming to me in the time it takes to type this sentence. 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: Merlin Moncure on 3 Feb 2010 11:41 On Wed, Feb 3, 2010 at 11:18 AM, Marko Tiikkaja <marko.tiikkaja(a)cs.helsinki.fi> wrote: > 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. > > > 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. Outside of documentation issues, where do we stand? Do you need help with the documentation? merlin -- 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:47 On 2010-02-03 18:41 UTC+2, Merlin Moncure wrote: > On Wed, Feb 3, 2010 at 11:18 AM, Marko Tiikkaja > <marko.tiikkaja(a)cs.helsinki.fi> wrote: >> 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. > Do you need help with the documentation? I'm going to work on the documentation tonight, but it will probably need some work from a native English speaker after I'm done. 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:49 On Wed, Feb 3, 2010 at 11:18 AM, Marko Tiikkaja <marko.tiikkaja(a)cs.helsinki.fi> wrote: >> 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. Name seems fine. Just fix the comment. >> 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. Well, technically good is certainly a good place to start. :-) Of course, we need the docs, too. Thanks for your work on this. ....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 |