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
