Re: EXPLAIN BUFFERS

From: Euler Taveira de Oliveira <euler(at)timbira(dot)com>
To: Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
Cc: Jeff Janes <jeff(dot)janes(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: EXPLAIN BUFFERS
Date: 2009-12-07 05:27:38
Message-ID: 4B1C924A.8000800@timbira.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Itagaki Takahiro escreveu:
> The attached patch is rebased to current CVS.
>
I'm looking at your patch now... It is almost there but has some issues.

(i) documentation: you have more than three counters and they could be
mentioned in docs too.

+ Include information on the buffers. Specifically, include the number of
+ buffer hits, number of disk blocks read, and number of local buffer read.

(ii) format: why does text output format have less counters than the other ones?

+ if (es->format == EXPLAIN_FORMAT_TEXT)
+ {
+ appendStringInfoSpaces(es->str, es->indent * 2);
+ appendStringInfo(es->str, "Blocks Hit: %ld Read: %ld Temp Read:
%ld\n",
+ usage->blks_hit, usage->blks_read, usage->temp_blks_read);
+ }
+ else
+ {
+ ExplainPropertyLong("Hit Blocks", usage->blks_hit, es);
+ ExplainPropertyLong("Read Blocks", usage->blks_read, es);
+ ExplainPropertyLong("Written Blocks", usage->blks_written, es);
+ ExplainPropertyLong("Local Hit Blocks", usage->local_blks_hit, es);
+ ExplainPropertyLong("Local Read Blocks", usage->local_blks_read, es);
+ ExplainPropertyLong("Local Written Blocks",
usage->local_blks_written, es);
+ ExplainPropertyLong("Temp Read Blocks", usage->temp_blks_read, es);
+ ExplainPropertyLong("Temp Written Blocks",
usage->temp_blks_written, es);
+ }

(iii) string: i don't like the string in text format because (1) it is not
concise (only the first item has the word 'Blocks'), (2) what block is it
about? Shared, Local, or Temp?, (3) why don't you include the other ones?, and
(4) why don't you include the written counters?

-> Seq Scan on pg_namespace nc (cost=0.00..1.07 rows=4 width=68) (actual
time=0.015..0.165 rows=4 loops=1)
Filter: (NOT pg_is_other_temp_schema(oid))
Blocks Hit: 11 Read: 0 Temp Read: 0

(iv) text format: i don't have a good suggestion but here are some ideas. The
former is too long and the latter is too verbose. :( Another option is to
suppress words hit, read, and written; and just document it.

Shared Blocks (11 hit, 5 read, 0 written); Local Blocks (5 hit, 0 read, 0
written); Temp Blocks (0 read, 0 written)

or

Shared Blocks: 11 hit, 5 read, 0 written
Local Blocks: 5 hit, 0 read, 0 written
Temp Blocks: 0 read, 0 written

(v) accumulative: i don't remember if we discussed it but is there a reason
the number of buffers isn't accumulative? We already have cost and time that
are both accumulative. I saw BufferUsageAccumDiff() function but didn't try to
figure out why it isn't accumulating or passing the counters to parent nodes.

euler=# explain (analyze true, buffers true) select * from pgbench_branches
inner join pgbench_history using (bid) where bid > 100;
QUERY PLAN

------------------------------------------------------------------------------------------------------------------------
Hash Join (cost=1.02..18.62 rows=3 width=476) (actual time=0.136..0.136
rows=0 loops=1)
Hash Cond: (pgbench_history.bid = pgbench_branches.bid)
Blocks Hit: 2 Read: 0 Temp Read: 0
-> Seq Scan on pgbench_history (cost=0.00..15.50 rows=550 width=116)
(actual time=0.034..0.034 rows=1 loops=1)
Blocks Hit: 1 Read: 0 Temp Read: 0
-> Hash (cost=1.01..1.01 rows=1 width=364) (actual time=0.022..0.022
rows=0 loops=1)
Blocks Hit: 0 Read: 0 Temp Read: 0
-> Seq Scan on pgbench_branches (cost=0.00..1.01 rows=1 width=364)
(actual time=0.019..0.019 rows=0 loops=1)
Filter: (bid > 100)
Blocks Hit: 1 Read: 0 Temp Read: 0
Total runtime: 0.531 ms
(11 rows)

(vi) comment: the 'at start' is superfluous. Please, remove it.

+ long blks_hit; /* # of buffer hits at start */
+ long blks_read; /* # of disk blocks read at start */

(vii) all nodes: I'm thinking if we need this information in all nodes (even
in those nodes that don't read or write). It would be less verbose but it
could complicate some parser's life. Of course, if we suppress this
information, we need to include it on the top node even if we don't read or
write in it.

I didn't have time to adjust your patch per comments above but if you can
address all of those issues I certainly could check your patch again.

--
Euler Taveira de Oliveira
http://www.timbira.com/

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Itagaki Takahiro 2009-12-07 05:31:31 Re: New VACUUM FULL
Previous Message Tom Lane 2009-12-07 05:26:42 Re: operator exclusion constraints