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