ProcessUtility_hook
in [PgSql]
|
From: Tom Lane on 30 Nov 2009 21:24 Bruce Momjian <bruce(a)momjian.us> writes: > 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? Well, that's for the reviewer to say --- if the update satisfies his concerns, he should sign off on it, if not not. I've tried to avoid pre-empting that process. > Also, we are two weeks into the commit fest and we have more unapplied > patches than applied ones. Yup. Lots of unfinished reviews out there. Robert spent a good deal of effort in the last two fests trying to light fires under reviewers; do you want to take up that cudgel? I think wholesale commits of things that haven't finished review is mostly going to send a signal that the review process doesn't matter, which is *not* the signal I think we should send. 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 21:32 OK, reverted and placed back in "Needs Review" status. --------------------------------------------------------------------------- Tom Lane wrote: > 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 -- 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: Bruce Momjian on 30 Nov 2009 21:36 Tom Lane wrote: > Bruce Momjian <bruce(a)momjian.us> writes: > > 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? > > Well, that's for the reviewer to say --- if the update satisfies his > concerns, he should sign off on it, if not not. I've tried to avoid > pre-empting that process. OK, so the reviewer knows he has to reply to the author's comments, OK. > > Also, we are two weeks into the commit fest and we have more unapplied > > patches than applied ones. > > Yup. Lots of unfinished reviews out there. Robert spent a good deal > of effort in the last two fests trying to light fires under reviewers; > do you want to take up that cudgel? I think wholesale commits of things I am afraid I am then duplicating work the commit fest manager is doing, and if someone is bugged by me and the CF manager, they might get upset. > that haven't finished review is mostly going to send a signal that the > review process doesn't matter, which is *not* the signal I think we > should send. True. Maybe I am best focusing on open issues like the threading and psql -1 patches I worked on today. There is certainly enough of that stuff to keep me busy. I thought I could help with the commit fest load, but now I am unsure. That non-commit-fest stuff has to be done too so maybe managing that will help. -- 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: Euler Taveira de Oliveira on 1 Dec 2009 06:52 Tom Lane escreveu: > Bruce Momjian <bruce(a)momjian.us> writes: >> 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? > > Well, that's for the reviewer to say --- if the update satisfies his > concerns, he should sign off on it, if not not. I've tried to avoid > pre-empting that process. > That's correct. I didn't have time to review the new patch yet. :( I'll do it later today. IIRC Tom had some objections (during the last CF) the way the patch was proposed and suggested changes. Let's see if the Takahiro-san did everything that was suggested. -- Euler Taveira de Oliveira http://www.timbira.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: Itagaki Takahiro on 3 Dec 2009 02:05 Tom Lane <tgl(a)sss.pgh.pa.us> wrote: > ... 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. I moved the initialization of completionTag into standard_ProcessUtility. > 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"? Ok, fixed. > 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. I removed the isTopLevel check. I was worried about auto-generated utility commands; generated sub commands are called with the same query string as the top query. Don't it confuse statistics? > 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. Ok, fixed. I've thought string-based interface is not desirable, but it should be a stable API. COPY and INSERT/UPDATE/DELETE (used by EXECUTE) are counted by pg_stat_statements, but EXECUTE SELECT is impossible. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center
First
|
Prev
|
Next
|
Last
Pages: 1 2 3 4 5 Prev: UTF8 with BOM support in psql Next: alpha1 bundled -- please verify |