Re: Buffer usage in EXPLAIN and pg_stat_statements (review)

Itagaki Takahiro escreveu:
> Thanks. Except choice of words, can I think the basic architecture of
> the patch is ok? The codes to handle global variables like ReadBufferCount
> and GlobalReadBufferCount could be rewrite in cleaner way if we could
> drop supports of log_{parser|planner|executor|statement}_stats.
>
Yes, it is. I'm afraid someone is relying on that piece of code. We can ask
people if it is ok to deprecated it; but it should be removed after 2 releases
or so. BTW, Isn't it make sense to move the Global* variables to buf_init.c,
is it?

> We should define the meanings of "get" and "hit" before rename them.
> I'd like to propose the meanings as following:
> * "get" : number of page access (= hit + read)
> * "hit" : number of cache read (no disk read)
> * "read" : number of disk read (= number of read() calls)
>
+1.

> But there are some confusions in postgres; ReadBufferCount and
> BufferHitCount are used for "get" and "hit", but "heap_blks_read"
> and "heap_blks_hit" are used for "read" and "hit" in pg_statio_all_tables.
I see. :(

> Can I rename ReadBufferCount to BufferGetCount? And which values should
> we show in EXPLAIN and pg_stat_statements? (two of the three are enough)
>
Do you want to include number of page access in the stats? If not, we don't
need to rename the variables for now (maybe a separate patch?). And IMHO we
should include "hit" and "read" because "get" is just a simple math.

> I borrowed the terms "Buffer Gets" and "Buffer Reads" from STATSPACK report
> in Oracle Database. But I'm willing to rename them if appropriate.
> http://www.oracle.com/apps_benchmark/doc/awrrpt_20090325b_900.html#600
>
Hmm... I don't have a strong opinion about those names as I said. So if you
want to revert the old names...

> Presently "Temp Buffers" contains temporary sort files, hash files, and
> materialized executor plan. Local buffer statistics for temp tables are
> not included here but merged with shared buffer statistics. Are there
> any better way to group them?
>
Are you sure? Looking at ReadBuffer_common() function I see an 'if
(isLocalBuf)' test.

As I said your patch is in good shape and if we solve those terminologies, it
is done for a committer.

Would you care to submit another patch if you want to do some modifications?

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