Re: pgBufferUsage.blk_{read|write}_time are zero although there are pgBufferUsage.local_blks_{read|written}

From: Nazir Bilal Yavuz <byavuz81(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Melanie Plageman <melanieplageman(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: pgBufferUsage.blk_{read|write}_time are zero although there are pgBufferUsage.local_blks_{read|written}
Date: 2023-10-17 13:44:25
Message-ID: CAN55FZ1==nG6d3WgqzWz_py1iV2ZiZQxDeVrXdJqkpOm7C+hsg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Thanks for the review!

On Tue, 17 Oct 2023 at 11:40, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Oct 16, 2023 at 01:07:07PM +0300, Nazir Bilal Yavuz wrote:
> > Yes, that could be a better solution. Also, having more detailed stats
> > for shared and local buffers is helpful. I updated patches in line
> > with that:
> >
> > 0001: Counts extends same way as a write.
>
> It can change existing query results on an already-released branch,
> but we already count the number of blocks when doing a relation
> extension, so counting the write time is something I'd rather fix in
> v16. If you have any objections, let me know.

I agree.

>
> > 0002: Rename blk_{read|write}_time as shared_blk_{read|write}_time.
>
> Note that `git diff --check` complains here.
>
> --- a/contrib/pg_stat_statements/pg_stat_statements--1.1--1.2.sql
> +++ b/contrib/pg_stat_statements/pg_stat_statements--1.1--1.2.sql
> @@ -30,8 +30,8 @@ CREATE FUNCTION pg_stat_statements(IN showtext boolean,
> OUT local_blks_written int8,
> OUT temp_blks_read int8,
> OUT temp_blks_written int8,
> - OUT blk_read_time float8,
> - OUT blk_write_time float8
> + OUT shared_blk_read_time float8,
> + OUT shared_blk_write_time float8
>
> Doing that in an extension upgrade script is incorrect. These should
> not be touched.
>
> - Total time the statement spent reading data file blocks, in milliseconds
> + Total time the statement spent reading shared data file blocks, in milliseconds
>
> Or just shared blocks? That's what we use elsewhere for
> pg_stat_statements. "shared data file blocks" sounds a bit confusing
> for relation file blocks read/written from/to shared buffers.
>
> > 0003: Add new local_blk_{read|write}_time variables.
>
> DATA = pg_stat_statements--1.4.sql \
> + pg_stat_statements--1.11--1.12.sql \
> pg_stat_statements--1.10--1.11.sql \
>
> There is no need to bump again pg_stat_statements, as it has already
> been bumped to 1.11 on HEAD per the recent commit 5a3423ad8ee1 from
> Daniel. So the new changes can just be added to 1.11.

I updated patches based on your comments. v4 is attached.

Regards,
Nazir Bilal Yavuz
Microsoft

Attachment Content-Type Size
v4-0001-Include-IOOp-IOOP_EXTENDs-while-calculating-block.patch text/x-diff 1.0 KB
v4-0002-Rename-I-O-timing-statistics-columns-to-shared_bl.patch text/x-diff 17.1 KB
v4-0003-Add-local_blk_-read-write-_time-I-O-timing-statis.patch text/x-diff 12.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2023-10-17 13:52:36 Re: run pgindent on a regular basis / scripted manner
Previous Message Imseih (AWS), Sami 2023-10-17 13:43:54 Re: False "pg_serial": apparent wraparound” in logs