Re: Buffer statistics for pg_stat_statements

We have infrastructure to count numbers buffer access in 8.5 Alpha 3.
I'd like to add per-query buffer usage into contrib/pg_stat_statements.

The pg_stat_statements view will have the same contents with
struct BufferUsage. Fields named shared_blks_{hit|read|written},
local_blks_{hit|read|written}, and temp_blks_{read|written}
will be added to the view.

We can determine slow queries not only based on durations but also
based on I/O or memory access count. Also, queries with non-zero
temp_blks_read means DBA need to consider increasing work_mem. Those
information would be useful to find where the server's bottleneck is.

Additional management costs cannot be avoided, but I think it should be
not so high because we accumulate buffer usage only once per query,
while EXPLAIN BUFFERS is slow because we need per-tuple calculation.

I'll submit this pg_stat_statements enhancement to the next commit fest.
Comments welcome.

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

Le vendredi 18 décembre 2009 09:44:40, Takahiro Itagaki a écrit :
> We have infrastructure to count numbers buffer access in 8.5 Alpha 3.
> I'd like to add per-query buffer usage into contrib/pg_stat_statements.
>
> The pg_stat_statements view will have the same contents with
> struct BufferUsage. Fields named shared_blks_{hit|read|written},
> local_blks_{hit|read|written}, and temp_blks_{read|written}
> will be added to the view.
>
> We can determine slow queries not only based on durations but also
> based on I/O or memory access count. Also, queries with non-zero
> temp_blks_read means DBA need to consider increasing work_mem. Those
> information would be useful to find where the server's bottleneck is.
>
> Additional management costs cannot be avoided, but I think it should be
> not so high because we accumulate buffer usage only once per query,
> while EXPLAIN BUFFERS is slow because we need per-tuple calculation.
>
> I'll submit this pg_stat_statements enhancement to the next commit fest.
> Comments welcome.

Very good idea.
Perhaps it can be usefull to have the percentage for hit/read ratio computed
in the view ?

>
> Regards,
> ---
> Takahiro Itagaki
> NTT Open Source Software Center
>

--
Cédric Villemain
Administrateur de Base de Données
Cel: +33 (0)6 74 15 56 53
http://dalibo.com - http://dalibo.org

Cedric Villemain <cedric(dot)villemain(at)dalibo(dot)com> wrote:

> Le vendredi 18 decembre 2009 09:44:40, Takahiro Itagaki a ecrit :
> > I'd like to add per-query buffer usage into contrib/pg_stat_statements.

Here is a patch to add buffer usage columns into pg_stat_statements.
It also changes initialzation of the result TupleDesc from manually
coded routines to get_call_result_type().

> Perhaps it can be usefull to have the percentage for hit/read ratio computed
> in the view ?

I think different DBAs want different derived values; Some of them might want
the buffer hit ratio, but others might request numbers per query. I'd like to
privide only raw values from pg_stat_statements to keep it simple, but I added
a computational expression of hit percentage in the documentation.

! bench=# SELECT query, calls, total_time, rows, 100.0 * shared_blks_hit /
! nullif(shared_blks_hit + shared_blks_read, 0) AS hit_percent
! FROM pg_stat_statements ORDER BY total_time DESC LIMIT 5;
! -[ RECORD 1 ]---------------------------------------------------------------------
! query | UPDATE pgbench_branches SET bbalance = bbalance + $1 WHERE bid = $2;
! calls | 3000
! total_time | 9.60900100000002
! rows | 2836
! hit_percent | 99.9778970000200936

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

2009/12/22 Takahiro Itagaki <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>:
>
> Cedric Villemain <cedric(dot)villemain(at)dalibo(dot)com> wrote:
>
>> Le vendredi 18 decembre 2009 09:44:40, Takahiro Itagaki a ecrit :
>> > I'd like to add per-query buffer usage into contrib/pg_stat_statements.
>
> Here is a patch to add buffer usage columns into pg_stat_statements.
> It also changes initialzation of the result TupleDesc from manually
> coded routines to get_call_result_type().
>
>> Perhaps it can be usefull to have the percentage for hit/read ratio computed
>> in the view ?
>
> I think different DBAs want different derived values; Some of them might want
> the buffer hit ratio, but others might request numbers per query. I'd like to
> privide only raw values from pg_stat_statements to keep it simple, but I added
> a computational expression of hit percentage in the documentation.

Yes, you are right.

>
> ! bench=# SELECT query, calls, total_time, rows, 100.0 * shared_blks_hit /
> !                nullif(shared_blks_hit + shared_blks_read, 0) AS hit_percent
> !           FROM pg_stat_statements ORDER BY total_time DESC LIMIT 5;
> ! -[ RECORD 1 ]---------------------------------------------------------------------
> ! query       | UPDATE pgbench_branches SET bbalance = bbalance + $1 WHERE bid = $2;
> ! calls       | 3000
> ! total_time  | 9.60900100000002
> ! rows        | 2836
> ! hit_percent | 99.9778970000200936
>
> Regards,
> ---
> Takahiro Itagaki
> NTT Open Source Software Center
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On Tue, Dec 22, 2009 at 3:27 AM, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Cedric Villemain <cedric(dot)villemain(at)dalibo(dot)com> wrote:
>> Le vendredi 18 decembre 2009 09:44:40, Takahiro Itagaki a ecrit :
>> > I'd like to add per-query buffer usage into contrib/pg_stat_statements.
>
> Here is a patch to add buffer usage columns into pg_stat_statements.
> It also changes initialzation of the result TupleDesc from manually
> coded routines to get_call_result_type().

I have reviewed this patch and I think it looks pretty good. A couple
of minor nits:

- There are needless whitespace changes in the definition of struct
Counters. The changes to the existing four members should be
reverted, and the new members should be made to match the existing
members.

- In the part that reads /* calc differences of buffer counters */,
all the lines go past 80 columns. I wonder if it would be better to
insert a line break just after the equals sign and indent the next
line by an extra tab stop. See, e.g. src/backend/commands/user.c line
338.

Other than that I think this is ready to commit.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I have reviewed this patch and I think it looks pretty good. A couple
> of minor nits:
>
> - There are needless whitespace changes in the definition of struct
> Counters. The changes to the existing four members should be
> reverted, and the new members should be made to match the existing
> members.

That's because the 'shared_blks_written' field is too long to keep the
existing indentations. Since we still have some rooms in 80 columns,
I'd like to change all of them as the previous patch.

> - In the part that reads /* calc differences of buffer counters */,
> all the lines go past 80 columns. I wonder if it would be better to
> insert a line break just after the equals sign and indent the next
> line by an extra tab stop. See, e.g. src/backend/commands/user.c line
> 338.

Ok, I'll adjust them so.

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

On Sun, Jan 3, 2010 at 10:17 PM, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
>> I have reviewed this patch and I think it looks pretty good.  A couple
>> of minor nits:
>>
>> - There are needless whitespace changes in the definition of struct
>> Counters.  The changes to the existing four members should be
>> reverted, and the new members should be made to match the existing
>> members.
>
> That's because the 'shared_blks_written' field is too long to keep the
> existing indentations. Since we still have some rooms in 80 columns,
> I'd like to change all of them as the previous patch.

I don't necessarily know what the right thing to do with the new ones
is, but I am pretty sure that pg_indent will revert any changes you
make to the existing ones. Assuming that's so, it doesn't make sense
to change them.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sun, Jan 3, 2010 at 10:17 PM, Takahiro Itagaki
> <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> - There are needless whitespace changes in the definition of struct
>>> Counters. The changes to the existing four members should be
>>> reverted, and the new members should be made to match the existing
>>> members.
>>
>> That's because the 'shared_blks_written' field is too long to keep the
>> existing indentations. Since we still have some rooms in 80 columns,
>> I'd like to change all of them as the previous patch.

> I don't necessarily know what the right thing to do with the new ones
> is, but I am pretty sure that pg_indent will revert any changes you
> make to the existing ones.

That it will. The proposed changes to the existing lines are an
exercise in uselessness; and to the extent that you format the added
lines with this layout in mind, the final result could be worse than
what you'd get if you adapt to pg_indent's rules to start with.

One possibility is to adopt shorter field names than these.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> > I don't necessarily know what the right thing to do with the new ones
> > is, but I am pretty sure that pg_indent will revert any changes you
> > make to the existing ones.
>
> That it will. The proposed changes to the existing lines are an
> exercise in uselessness; and to the extent that you format the added
> lines with this layout in mind, the final result could be worse than
> what you'd get if you adapt to pg_indent's rules to start with.

Here is the proposed patch to adjust white spaces.
It does not indent variables, but indents comments of the variables
to adjust other fields. Are those changes ok?

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

On Thu, Jan 7, 2010 at 3:31 AM, Takahiro Itagaki
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> > I don't necessarily know what the right thing to do with the new ones
>> > is, but I am pretty sure that pg_indent will revert any changes you
>> > make to the existing ones.
>>
>> That it will.  The proposed changes to the existing lines are an
>> exercise in uselessness; and to the extent that you format the added
>> lines with this layout in mind, the final result could be worse than
>> what you'd get if you adapt to pg_indent's rules to start with.
>
> Here is the proposed patch to adjust white spaces.
> It does not indent variables, but indents comments of the variables
> to adjust other fields. Are those changes ok?

I think so. I'm not sure if it will push out the comment that is
immediately adjacent to the trailing semicolon, but I don't think it
will decrease the indent on the ones you've indented more. I think
this is close enough for now and you should go ahead and commit it.

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I think so. I'm not sure if it will push out the comment that is
> immediately adjacent to the trailing semicolon, but I don't think it
> will decrease the indent on the ones you've indented more. I think
> this is close enough for now and you should go ahead and commit it.

Thanks for your review. I committed it.

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