Re: ProcessUtility_hook

I applied this patch after a little bit of editorialization concerning
the points mentioned in this discussion:

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Dec 9, 2009 at 9:33 PM, Takahiro Itagaki
> <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)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