Date: Tue, 5 Apr 2022 11:57:14 +0800
From: Julien Rouhaud <rjuju123@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: gkokolatos@pm.me, Ranier Vilela <ranier.vf@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	depesz@depesz.com
Subject: Re: Showing I/O timings spent reading/writing temp buffers in EXPLAIN
Message-ID: <Yku+GopF86WjF26r@jrouhaud>
References: 
 <CAD21AoAdwWBxBn8YQucsRGEB3ToYH4C6wjGU+qajKLmiJDCJkw@mail.gmail.com>
 <CAEudQAogZVHURE7fSfbOmOZMPK7tR+FMR4b=5WKVU7BersxaOQ@mail.gmail.com>
 <2sAuEW1zGE4cTWCRPgzvAshfUAM44kW02tc3i31iLLCujLE-tczA8xOGFhs6c2EiEVM5uSQy_cJyvgAA1EKyxRtqnWM_mrirKwsTJ6Kxaaw=@pm.me>
 <CAD21AoDXgYRVrzWpAV_CoX0nbg68++Bx4RbkzY40hKm5h1mEtg@mail.gmail.com>
 <20220119085236.iuuq4rtzm6qxvzls@jrouhaud>
 <CAD21AoAca6EdwRhhqbLuAsOxMsKKZB9qMX_m9LenLS2NE6fM1A@mail.gmail.com>
 <Ykp0XP37Gj/sT5hB@jrouhaud>
 <CAD21AoAsOhTetfaDbrpmp7Xgb=E6RHkPy1R9qvMUA8TJzEBR5g@mail.gmail.com>
 <YksdaopBu5CiOoFk@jrouhaud>
 <CAD21AoDDXfhRsb28FgKGOFdm-Ayeb+yFSSDW-oopwY8ydQyjAA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAD21AoDDXfhRsb28FgKGOFdm-Ayeb+yFSSDW-oopwY8ydQyjAA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/Yku%2BGopF86WjF26r%40jrouhaud>
Precedence: bulk

On Tue, Apr 05, 2022 at 10:40:04AM +0900, Masahiko Sawada wrote:
> On Tue, Apr 5, 2022 at 1:31 AM Julien Rouhaud <rjuju123@gmail.com> wrote:
> >
> > Yes.  In normal circumstances it shouldn't need a lot of time to do that, but
> > I'm not so sure with e.g. network filesystems.  I'm not strongly in favor of
> > counting it, especially since smgrextend doesn't either.
> 
> Good point. I think that adding a new place to track I/O timing can be
> a separate patch so probably we can work on it for PG16 or later.

Agreed.

> I've attached updated patches, please review it.

It looks good to me, just one minor thing in 002:

@@ -183,8 +184,10 @@ typedef struct Counters
 	int64		local_blks_written; /* # of local disk blocks written */
 	int64		temp_blks_read; /* # of temp blocks read */
 	int64		temp_blks_written;	/* # of temp blocks written */
-	double		blk_read_time;	/* time spent reading, in msec */
-	double		blk_write_time; /* time spent writing, in msec */
+	double		blk_read_time;	/* time spent reading blocks, in msec */
+	double		blk_write_time; /* time spent writing blocks, in msec */
+	double		temp_blk_read_time;	/* time spent reading temp blocks, in msec */
+	double		temp_blk_write_time; /* time spent writing temp blocks, in msec */

maybe the comments should respectively be data file blocks and temp file
blocks.

This is a minor detail and the rest of the patch looks good to me, so I'm
marking the patch as Ready for Committer!