ProcessUtility_hook
in [PgSql]
|
From: Robert Haas on 9 Dec 2009 22:03 On Wed, Dec 9, 2009 at 9:33 PM, Takahiro Itagaki <itagaki.takahiro(a)oss.ntt.co.jp> wrote: > Robert Haas <robertmhaas(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Takahiro Itagaki on 9 Dec 2009 22:14 Robert Haas <robertmhaas(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 9 Dec 2009 22:20 On Wed, Dec 9, 2009 at 10:14 PM, Takahiro Itagaki <itagaki.takahiro(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Tom Lane on 15 Dec 2009 15:15 I applied this patch after a little bit of editorialization concerning the points mentioned in this discussion: Robert Haas <robertmhaas(a)gmail.com> writes: > On Wed, Dec 9, 2009 at 9:33 PM, Takahiro Itagaki > <itagaki.takahiro(a)oss.ntt.co.jp> wrote: >> Robert Haas <robertmhaas(a)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(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
First
|
Prev
|
Pages: 1 2 3 4 5 Prev: UTF8 with BOM support in psql Next: alpha1 bundled -- please verify |