ProcessUtility_hook
in [PgSql]
|
From: Tom Lane on 30 Nov 2009 20:33 Bruce Momjian <bruce(a)momjian.us> writes: > I have applied this patch, with only a minor wording improvement: Uh, we weren't even done reviewing this were we? 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: Bruce Momjian on 30 Nov 2009 20:35 Tom Lane wrote: > Bruce Momjian <bruce(a)momjian.us> writes: > > I have applied this patch, with only a minor wording improvement: > > Uh, we weren't even done reviewing this were we? Uh, I am new to this commitfest wiki thing, but it did have a review by Euler Taveira de Oliveira: https://commitfest.postgresql.org/action/patch_view?id=196 and the author replied. Is there more that needs to be done? -- Bruce Momjian <bruce(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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 30 Nov 2009 20:38 Bruce Momjian <bruce(a)momjian.us> writes: > Tom Lane wrote: >> Uh, we weren't even done reviewing this were we? > Uh, I am new to this commitfest wiki thing, but it did have a review by > Euler Taveira de Oliveira: > https://commitfest.postgresql.org/action/patch_view?id=196 > and the author replied. Is there more that needs to be done? It wasn't marked Ready For Committer, so presumably the reviewer wasn't done with it. I know I hadn't looked at it at all, because I was waiting for the commitfest review process to finish. 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: Bruce Momjian on 30 Nov 2009 20:55 Tom Lane wrote: > Bruce Momjian <bruce(a)momjian.us> writes: > > Tom Lane wrote: > >> Uh, we weren't even done reviewing this were we? > > > Uh, I am new to this commitfest wiki thing, but it did have a review by > > Euler Taveira de Oliveira: > > https://commitfest.postgresql.org/action/patch_view?id=196 > > and the author replied. Is there more that needs to be done? > > It wasn't marked Ready For Committer, so presumably the reviewer > wasn't done with it. I know I hadn't looked at it at all, because > I was waiting for the commitfest review process to finish. So, if someone writes a patch, and it is reviewed, and the patch author updates the patch and replies, it still should be reviewed again before being committed? I was unclear on that. The updated patch only appeared today, so maybe it was ready, but the commit fest manager has to indicate that in the status before I review/apply it? Should I revert the patch? So there is nothing for me to do to help? The only two patches I see as ready for committer are HS and SR; not going to touch those. ;-) Also, we are two weeks into the commit fest and we have more unapplied patches than applied ones. -- Bruce Momjian <bruce(a)momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- 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 30 Nov 2009 21:06 I wrote: > It wasn't marked Ready For Committer, so presumably the reviewer > wasn't done with it. I know I hadn't looked at it at all, because > I was waiting for the commitfest review process to finish. .... and now that I have, I find at least four highly questionable things about it: 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. 2. The naming and documentation of the added GUC setting for pg_stat_statements. "track_ddl" seems pretty bizarre to me because there are many utility statements that no one would call DDL. COPY, for example, is certainly not DDL. Why not call it "track_utility"? 3. The enable-condition test in pgss_ProcessUtility. Is it really appropriate to be gating this by isTopLevel? I should think that the nested_level check in pgss_enabled would be sufficient and more likely to do what's expected. 4. The special case for CopyStmt. That's just weird, and it adds a maintenance requirement we don't need. I don't see a really good argument why COPY (alone among utility statements) deserves to have a rowcount tracked by pg_stat_statements, but even if you want that it'd be better to rely on examining the completionTag after the fact. The fact that the tag is "COPY nnnn" is part of the user-visible API for COPY and won't change lightly. The division of labor between ProcessUtility and copy.c is far more volatile, but this patch has injected itself into that. 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
First
|
Prev
|
Next
|
Last
Pages: 1 2 3 4 5 Prev: UTF8 with BOM support in psql Next: alpha1 bundled -- please verify |