On Sun, Oct 4, 2009 at 11:22 PM, Itagaki Takahiro <itagaki.takah...@oss.ntt.co.jp> wrote: > Robert Haas <robertmh...@gmail.com> wrote: > >> 1. I would suggest something like "Blocks >> Read: %ld Hit: %ld Temp Read: %ld\n". See the way we handle output >> of sort type and space usage, for example. > > I have some questions: > * Did you use single space and double spaces in your example intentionally?
No, that was unintentional. > * Should we use lower cases here? No. We don't anywhere else in explain.c. > * Can I use "temp" instead of "Temp Read" to shorten the name? I can't tell what that means without reading the source code. I think clarity should take precedence over brevity. >> 2. Similarly, in pg_stat_statements, the Counters structure could >> easily use the same names for the structure members that we already >> use in e.g. pg_stat_database - blks_hit, blks_read, and, say, >> blks_temp_read. In fact I tend to think we should stick with "blocks" >> rather than "buffers" overall, for consistency with what the system >> does elsewhere. > > I agree to rename them into blks_*, but EXPLAIN (blocks) might be > misleading; EXPLAIN (buffer) can be interpreted as "buffer usage", > but normally we don't call it "block usage". > > My suggestion is: > * EXPLAIN (buffers) prints (blocks read: %ld hit: %ld temp: %ld) > * auto_explain.log_buffers are not changed > * pg_stat_statements uses blks_hit and blks_read I agree. >> 4. "Instrumentation stack is broken" doesn't seem terribly helpful in >> understanding what has gone wrong. > > This message is only for hackers and should not occur. > Assert() might be ok instead. Hmm, I think I like the idea of an Assert(). Logging a cryptic message at DEBUG2 doesn't seem sufficient for a can't-happen condition that probably indicates a serious bug in the code. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
