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

Hi Itagaki-san,

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.

>   static bool auto_explain_log_analyze = false;
>   static bool auto_explain_log_verbose = false;
> + static bool auto_explain_log_buffer = false;
Rename it to auto_explain_log_buffers. That's because I renamed the option for
plural form. See above.

>               es.verbose = auto_explain_log_verbose;
> +             es.buffer = auto_explain_log_buffer;
Change this check to look at es.analyze too. So the es.buffers will only be
enabled iif the es.analyze is enabled too.


> +     /* 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?

>           else if (strcmp(opt->defname, "costs") == 0)
>               es.costs = defGetBoolean(opt);
> +         else if (strcmp(opt->defname, "buffer") == 0)
> +             es.buffer = defGetBoolean(opt);
I decided to change "buffer" to "buffers". That's because we already have
"costs" and the statistics will not be about one kind of buffer so plural form
sounds more natural.

> +             if (es->format == EXPLAIN_FORMAT_TEXT)
> +             {
> +                 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.

> +             }
> +             else
> +             {
> +                 ExplainPropertyLong("Buffer Gets", num_gets, es);
> +                 ExplainPropertyLong("Buffer Reads", num_reads, es);
> +                 ExplainPropertyLong("Buffer Temp", num_temp, es);
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"?

>   #include "parser/parsetree.h"
> + #include "storage/buf_internals.h"
It's not used. Removed.

> +         CurrentInstrument = instr->prev;
> +     }
> +     else
> +         elog(WARNING, "Instrumentation stack is broken");
WARNING? I changed it to DEBUG2 and return immediately (as it does some lines
of code above).

> + /* for log_[parser|planner|executor|statement]_stats */
> + static long GlobalReadBufferCount;
> + static long GlobalReadLocalBufferCount;
> + static long GlobalBufferHitCount;
> + static long GlobalLocalBufferHitCount;
> + static long GlobalBufferFlushCount;
> + static long GlobalLocalBufferFlushCount;
> + static long GlobalBufFileReadCount;
> + static long GlobalBufFileWriteCount;
> +
I'm not sure if this is the right place for these counters. Maybe we should
put it in buf_init.c. Ideas?

>       bool        costs;            /* print costs */
> +     bool        buffer;            /* print buffer stats */
Rename it to "buffers".

> +     /* Buffer usage */
> +     long        buffer_gets;    /* # of buffer reads */
> +     long        buffer_reads;    /* # of disk reads */
> +     long        buffer_temp;    /* # of temp file reads */
Rename them to "buffers_hit", "buffers_read", and "buffers_temp".

I didn't test this changes with "big" queries because I don't have some at
hand right now. Also, I didn't notice any slowdowns caused by the patch.
Comments? If none, it is ready for a committer.


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