Review of Writeable CTE Patch
in [PgSql]
|
From: Robert Haas on 26 Jan 2010 09:54 On Tue, Jan 26, 2010 at 9:13 AM, Merlin Moncure <mmoncure(a)gmail.com> wrote: > *) Works as advertised...'feels right'. Found only one small issue > which Marko was already aware of and had adjusted for. [...] > Marko was already aware of it and has a fix ready. So it sounds like we should expect an updated patch shortly? ....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: Marko Tiikkaja on 26 Jan 2010 10:06 On 2010-01-26 16:54, Robert Haas wrote: > On Tue, Jan 26, 2010 at 9:13 AM, Merlin Moncure <mmoncure(a)gmail.com> wrote: >> *) Works as advertised...'feels right'. Found only one small issue >> which Marko was already aware of and had adjusted for. > [...] >> Marko was already aware of it and has a fix ready. > > So it sounds like we should expect an updated patch shortly? Yes. I'm adding more comments and documentation, expect a patch within a couple of days. 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: Alvaro Herrera on 26 Jan 2010 10:11 Merlin Moncure escribi�: > *) CopySnapshot was promoted from static. Is this legal/good idea? > Is a wrapper more appropriate? Hmm ... I wonder why isn't the patch doing RegisterSnapshot with the passed snapshot directly -- why is it necessary to create a new copy of it? (I notice that only one of the arms in that "if" creates a copy; if that is correct, I think it warrants a comment explaining why). If the copy is necessary (e.g. because the snapshot must not be modified externally, and there's actual risk that it is), then maybe it would be better to create a new function RegisterSnapshotCopy instead? -- Alvaro Herrera http://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- 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 26 Jan 2010 10:16 On 2010-01-26 17:11, Alvaro Herrera wrote: > Merlin Moncure escribi�: > >> *) CopySnapshot was promoted from static. Is this legal/good idea? >> Is a wrapper more appropriate? > > Hmm ... I wonder why isn't the patch doing RegisterSnapshot with the > passed snapshot directly -- why is it necessary to create a new copy of > it? (I notice that only one of the arms in that "if" creates a copy; > if that is correct, I think it warrants a comment explaining why). Per discussion here: http://archives.postgresql.org/pgsql-hackers/2009-11/msg01964.php the executor copies the snapshot if it plans on modifying it. A comment explaining this might be in order. > If the copy is necessary (e.g. because the snapshot must not be modified > externally, and there's actual risk that it is), then maybe it would be > better to create a new function RegisterSnapshotCopy instead? Sounds reasonable. 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: Takahiro Itagaki on 3 Feb 2010 04:04
Marko Tiikkaja <marko.tiikkaja(a)cs.helsinki.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers |