Skip site navigation (1) Skip section navigation (2)

Re: Buffer usage in EXPLAIN and pg_stat_statements (review)

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: Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Buffer usage in EXPLAIN and pg_stat_statements (review)
Date: 2009-09-29 02:41:45
Message-ID: 4AC173E9.4010405@timbira.com (view raw or flat)
Thread:
Lists: 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/

In response to

Responses

pgsql-hackers by date

Next:From: Tom LaneDate: 2009-09-29 02:44:30
Subject: Re: [PATCH] DefaultACLs
Previous:From: Robert HaasDate: 2009-09-29 02:27:16
Subject: Re: [PATCH] DefaultACLs

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group