Re: [HACKERS] ProcessUtility_hook
I applied this patch after a little bit of editorialization concerning the points mentioned in this discussion: Robert Haas robertmh...@gmail.com writes: On Wed, Dec 9, 2009 at 9:33 PM, Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp wrote: Robert Haas robertmh...@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. I guess my vote is for leaving it alone, but I might not know what I'm talking about. :-) I removed this change. I'm not convinced that taking out _PG_fini is appropriate in the first place, but if it is, we should do it in all contrib modules together, not just randomly as side-effects of unrelated patches. 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. */ I took this out, too. It's inconsistent and IMHO the wrong thing anyway. If a regular DML statement is EXECUTE'd, the associated rowcounts will be tallied by the executor hooks, so this was double-counting the effects. The only way it wouldn't be double-counting is if you have tracking of nested statements turned off; but if you do, I don't see why you'd expect them to get counted anyway in this one particular case. 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. Nope, it didn't. The point here is that one of the purposes of a hook is to allow a loadable module to editorialize on the behavior of the hooked function, and that read-only check is part of the behavior of ProcessUtility. I doubt that bypassing the test would be a particularly smart thing for somebody to do, but it is for example reasonable to want to throw a warning or do nothing at all instead of allowing the error to occur. So that should be inside standard_ProcessUtility. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Robert Haas robertmh...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
On Wed, Dec 9, 2009 at 9:33 PM, Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp wrote: Robert Haas robertmh...@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. I guess my vote is for leaving it alone, but I might not know what I'm talking about. :-) 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. */ I'm confused by the we cannot retrieve the number of rows for SELECT part. Can you clarify that? 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? Beats me. I interpreted Tom's remark to be saying the hook call should come first, but I'm not sure which way is actually better. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Robert Haas robertmh...@gmail.com wrote: 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. */ I'm confused by the we cannot retrieve the number of rows for SELECT part. Can you clarify that? Ah, I meant the SELECT was EXECUTE of SELECT. If I use internal structure names, the explanation will be: EXECUTE command returns INSERT, UPDATE, DELETE, or SELECT tags. We can retrieve the number of rows from INSERT, UPDATE, and DELETE tags, but cannot from SELECT tag because the tag doesn't contain row numbers and also EState-es_processed is unavailable for EXECUTE commands. Regards, --- Takahiro Itagaki NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
On Wed, Dec 9, 2009 at 10:14 PM, Takahiro Itagaki itagaki.takah...@oss.ntt.co.jp wrote: I'm confused by the we cannot retrieve the number of rows for SELECT part. Can you clarify that? Ah, I meant the SELECT was EXECUTE of SELECT. If I use internal structure names, the explanation will be: EXECUTE command returns INSERT, UPDATE, DELETE, or SELECT tags. We can retrieve the number of rows from INSERT, UPDATE, and DELETE tags, but cannot from SELECT tag because the tag doesn't contain row numbers and also EState-es_processed is unavailable for EXECUTE commands. OK, that makes sense. It might read a little better this way: The EXECUTE command returns INSERT, UPDATE, DELETE, or SELECT tags. We can retrieve the number of rows from INSERT, UPDATE, and DELETE tags, but the SELECT doesn't contain row numbers. We also can't get it from EState-es_processed, because that is unavailable for EXECUTE commands. That seems like a rather unfortunate limitation though... ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
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 Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
On Tue, Dec 8, 2009 at 10:12 PM, Greg Smith g...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
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 Smith2ndQuadrant Baltimore, MD PostgreSQL Training, Services and Support g...@2ndquadrant.com www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Tom Lane t...@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 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 ProcessUtility_hook_20091203.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Tom Lane escreveu: Bruce Momjian br...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
I have applied this patch, with only a minor wording improvement: Specify literalon/ to track DDL commands, which excludes commandSELECT/, ^^ Thanks. --- Itagaki Takahiro wrote: Euler Taveira de Oliveira eu...@timbira.com wrote: The functionality is divided in two parts. The first part is a hook in the utility module. The idea is capture the commands that doesn't pass through executor. I'm afraid that that hook will be used only for capturing non-DML queries. If so, why don't we hack the tcop/postgres.c and grab those queries from the same point we log statements? DDLs can be used from user defined functions. It has the same reason why we have executor hooks instead of tcop hooks. The second part is to use that hook to capture non-DML commands for pg_stat_statements module. - I fixed a bug that it should handle only isTopLevel command. - A new parameter pg_stat_statements.track_ddl (boolean) is added to enable or disable the feature. Do we need to have rows = 0 for non-DML commands? Maybe NULL could be an appropriate value. Yes, but it requires additional management to handle 0 and NULL separately. I don't think it is worth doing. The PREPARE command stopped to count the number of rows. Should we count the rows in EXECUTE command or in the PREPARE command? It requires major rewrites of EXECUTE command to pass the number of affected rows to caller. I doubt it is worth fixing because almost all drivers use protocol-level prepared statements instead of PREPARE+EXECUTE. The other command that doesn't count properly is COPY. Could you fix that? I added codes for it. I'm concerned about storing some commands that expose passwords like CREATE ROLE foo PASSWORD 'secret'; I know that the queries are only showed to superusers but maybe we should add this information to documentation or provide a mechanism to exclude those commands. I think there is no additonal problem there because we can see the 'secret' command using pg_stat_activity even now. I don't know if it is worth the trouble adding some code to capture VACUUM and ANALYZE commands called inside autovacuum. I'd like to exclude VACUUM and ANALYZE by autovacuum because pg_stat_statements should not filled with those commands. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center [ Attachment, skipping... ] -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian br...@momjian.ushttp://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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Bruce Momjian br...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Bruce Momjian br...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
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 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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Bruce Momjian br...@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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
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 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 br...@momjian.ushttp://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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Tom Lane wrote: Bruce Momjian br...@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 br...@momjian.ushttp://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@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Euler Taveira de Oliveira eu...@timbira.com wrote: The functionality is divided in two parts. The first part is a hook in the utility module. The idea is capture the commands that doesn't pass through executor. I'm afraid that that hook will be used only for capturing non-DML queries. If so, why don't we hack the tcop/postgres.c and grab those queries from the same point we log statements? DDLs can be used from user defined functions. It has the same reason why we have executor hooks instead of tcop hooks. The second part is to use that hook to capture non-DML commands for pg_stat_statements module. - I fixed a bug that it should handle only isTopLevel command. - A new parameter pg_stat_statements.track_ddl (boolean) is added to enable or disable the feature. Do we need to have rows = 0 for non-DML commands? Maybe NULL could be an appropriate value. Yes, but it requires additional management to handle 0 and NULL separately. I don't think it is worth doing. The PREPARE command stopped to count the number of rows. Should we count the rows in EXECUTE command or in the PREPARE command? It requires major rewrites of EXECUTE command to pass the number of affected rows to caller. I doubt it is worth fixing because almost all drivers use protocol-level prepared statements instead of PREPARE+EXECUTE. The other command that doesn't count properly is COPY. Could you fix that? I added codes for it. I'm concerned about storing some commands that expose passwords like CREATE ROLE foo PASSWORD 'secret'; I know that the queries are only showed to superusers but maybe we should add this information to documentation or provide a mechanism to exclude those commands. I think there is no additonal problem there because we can see the 'secret' command using pg_stat_activity even now. I don't know if it is worth the trouble adding some code to capture VACUUM and ANALYZE commands called inside autovacuum. I'd like to exclude VACUUM and ANALYZE by autovacuum because pg_stat_statements should not filled with those commands. Regards, --- ITAGAKI Takahiro NTT Open Source Software Center ProcessUtility_hook_20091130.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Itagaki Takahiro escreveu: Here is a patch to add ProcessUtility_hook to handle all DDL commands in user modules. (ProcessUtility_hook_20091021.patch) It adds a global variable 'ProcessUtility_hook' and export the original function as 'standard_ProcessUtility'. I've reviewed your patch. It applies cleanly and compiles without warnings. It lacks documentation. Could you update it? The functionality is divided in two parts. The first part is a hook in the utility module. The idea is capture the commands that doesn't pass through executor. I'm afraid that that hook will be used only for capturing non-DML queries. If so, why don't we hack the tcop/postgres.c and grab those queries from the same point we log statements? This feature is similar to the executor one. But in the executor case, we use the plan for other things. Other utilities are (i) using that hook to filter out some non-DML commands and (ii) developing some non-DML replication mechanism for trigger-based replication solutions. The second part is to use that hook to capture non-DML commands for pg_stat_statements module. Do we need to have rows = 0 for non-DML commands? Maybe NULL could be an appropriate value. The PREPARE command stopped to count the number of rows. Should we count the rows in EXECUTE command or in the PREPARE command? The other command that doesn't count properly is COPY. Could you fix that? I'm concerned about storing some commands that expose passwords like CREATE ROLE foo PASSWORD 'secret'; I know that the queries are only showed to superusers but maybe we should add this information to documentation or provide a mechanism to exclude those commands. I don't know if it is worth the trouble adding some code to capture VACUUM and ANALYZE commands called inside autovacuum. -- Euler Taveira de Oliveira http://www.timbira.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Itagaki Takahiro escreveu: We can hook SELECT, INSERT, UPDATE and DELETE with ExecutorXxx_hooks, but there is no way to hook DDL commands. Can I submit a patch to add ProcessUtility_hook? +1. Hey, I was thinking about that yesterday. ;) pg_stat_statements module also handle DDL commands as same as normal queries. Fortunately, autovacuum calls call vacuum() directly, so the statistics will not filled with VACUUM and ANALYZE commands by autovacuum. I don't look at the code but maybe there is a way to hook something there too. But I'd leave it alone for now, unless it's few lines of code. -- Euler Taveira de Oliveira http://www.timbira.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ProcessUtility_hook
Here is a patch to add ProcessUtility_hook to handle all DDL commands in user modules. (ProcessUtility_hook_20091021.patch) It adds a global variable 'ProcessUtility_hook' and export the original function as 'standard_ProcessUtility'. Euler Taveira de Oliveira eu...@timbira.com wrote: pg_stat_statements module also handle DDL commands as same as normal queries. Fortunately, autovacuum calls call vacuum() directly, so the statistics will not filled with VACUUM and ANALYZE commands by autovacuum. I don't look at the code but maybe there is a way to hook something there too. But I'd leave it alone for now, unless it's few lines of code. I see it is debatable whether pg_stat_statements should support the hook. So I split the change in another patch. (pgss_utility_hook_20091021.patch) Regards, --- ITAGAKI Takahiro NTT Open Source Software Center ProcessUtility_hook_20091021.patch Description: Binary data pgss_utility_hook_20091021.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
