ProcessUtility_hook
in [PgSql]
|
From: Greg Smith on 8 Dec 2009 22:12 It looks like the last round of issues on this patch only came from Tom's concerns, so I'm not sure if anyone but Tom (or a similarly picky alternate committer) is going to find anything else in a re-review. It looks to me like all the issues were sorted out anyway. Euler, you're off the hook for this one; it looks "ready for committer" to me. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.com -- 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 8 Dec 2009 22:49 On Tue, Dec 8, 2009 at 10:12 PM, Greg Smith <greg(a)2ndquadrant.com> wrote: > It looks like the last round of issues on this patch only came from Tom's > concerns, so I'm not sure if anyone but Tom (or a similarly picky alternate > committer) is going to find anything else in a re-review. It looks to me > like all the issues were sorted out anyway. Euler, you're off the hook for > this one; it looks "ready for committer" to me. Since Itagaki Takahiro is now a committer, I sort of assumed he would be committing his own patches. I am not really sure what the etiquette is in this area, but there seems to be an implication here someone else will be committing this, which isn't necessarily my understanding of how it works. Certainly, unless someone has a contrary opinion, I think he can go ahead if he wishes. On the other hand, if it's helpful, I'm more than happy to pick up this one and/or EXPLAIN BUFFERS for final review and commit. ....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: Greg Smith on 8 Dec 2009 23:05 Robert Haas wrote: > Since Itagaki Takahiro is now a committer, I sort of assumed he would > be committing his own patches. Maybe, but I wasn't going to be the one to suggest that Tom get cut out of the loop after he raised a list of issues with the patch already. I think the situation for EXPLAIN BUFFERS is much simpler, given that the last round of feedback was only quibbling over the line formatting and docs. What I think is a reasonable general guideline is to route submitted patches back to their author to commit only when the patch has been recently free of code issues during its review. If we've already had another committer chime in with concerns, they should probably confirm themselves that the updated version is suitable to commit, and do so instead of the author. That just seems like a reasonable risk-reduction workflow approach to me, similar to how the "sign-off" practice works on some other projects. -- Greg Smith 2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support greg(a)2ndQuadrant.com www.2ndQuadrant.com -- 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 9 Dec 2009 21:15 Why does this patch #ifdef out the _PG_fini code in pg_stat_statements? Where you check for INSERT, UPDATE, and DELETE return codes in pgss_ProcessUtility, I think this deserves a comment explaining that these could occur as a result of EXECUTE. It wasn't obvious to me, anyway. It seems to me that the current hook placement does not address this complaint >> 1. The placement of the hook. Why is it three lines down in >> ProcessUtility? It's probably reasonable to have the Assert first, >> but I don't see why the hook function should have the ability to >> editorialize on the behavior of everything about ProcessUtility >> *except* the read-only-xact check. ....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: Takahiro Itagaki on 9 Dec 2009 21:33 Robert Haas <robertmhaas(a)gmail.com> wrote: > Why does this patch #ifdef out the _PG_fini code in pg_stat_statements? That's because _PG_fini is never called in current releases. We could remove it completely, but I'd like to leave it for future releases where _PG_fini callback is re-enabled. Or, removing #ifdef (enabling _PG_fini function) is also harmless. > Where you check for INSERT, UPDATE, and DELETE return codes in > pgss_ProcessUtility, I think this deserves a comment explaining that > these could occur as a result of EXECUTE. It wasn't obvious to me, > anyway. Like this? /* * Parse command tag to retrieve the number of affected rows. * COPY command returns COPY tag. EXECUTE command might return INSERT, * UPDATE, or DELETE tags, but we cannot retrieve the number of rows * for SELECT. We assume other commands always return 0 row. */ > It seems to me that the current hook placement does not address this complaint > >> I don't see why the hook function should have the ability to > >> editorialize on the behavior of everything about ProcessUtility > >> *except* the read-only-xact check. I moved the initialization code of completionTag as the comment, but check_xact_readonly() should be called before the hook. Am I missing something? 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
First
|
Prev
|
Next
|
Last
Pages: 1 2 3 4 5 Prev: UTF8 with BOM support in psql Next: alpha1 bundled -- please verify |