Re: Buffer usage in EXPLAIN and pg_stat_statements (review) - Mailing list pgsql-hackers

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/