Re: Buffer usage in EXPLAIN and pg_stat_statements (review)

Euler Taveira de Oliveira <euler@timbira.com> wrote:

> I'm reviewing your patch. Your patch is in good shape. It applies cleanly. All
> of the things are built as intended (including the two contrib modules). It
> doesn't include docs but I wrote it. Basically, I produced another patch (that
> are attached) correcting some minor gripes; docs are included too. The
> comments are in-line.

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.

> > +     /* Build a tuple descriptor for our result type */
> > +     if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> > +         elog(ERROR, "return type must be a row type");
> > + 
> >       per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
> >       oldcontext = MemoryContextSwitchTo(per_query_ctx);
> >   
> > !     tupdesc = CreateTupleDescCopy(tupdesc);
> >   
> Out of curiosity, any reason for this change?

That's because the new code is cleaner, I think. Since the result tuple
type is defined in OUT parameters, we don't have to re-define the result
with CreateTemplateTupleDesc().

> > +                 appendStringInfo(es->str, " (gets=%ld reads=%ld temp=%ld)",
> > +                     num_gets, num_reads, num_temp);
> Rename "gets" and "reads" to "hit" and "read". Maybe we could prefix it with
> "buf_" or something else.
> 
> Rename "num_gets" and "num_reads" to "num_hit" and "num_read". The later
> terminology is used all over the code.

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
cacheread (no disk read)   * "read" : number of disk read (= number of read() calls)
 

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.
Can I rename ReadBufferCount to BufferGetCount? And which values should
we show in EXPLAIN and pg_stat_statements? (two of the three are enough)

> I didn't like these terminologies. I came up with "Hit Buffers", "Read
> Buffers", and "Temp Buffers". I confess that I don't like the last ones.
> "Read Buffers"? We're reading from disk blocks. "Read Blocks"? "Read Disk
> Blocks"? "Read Data Blocks"?
> "Temp Buffers"? It could be temporary sort files, hash files (?), or temporary
> relations. "Hit Local Buffers"? "Local Buffers"? "Hit Temp Buffers"?

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

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?

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center