Re: ProcessUtility_hook - Mailing list pgsql-hackers

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

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