Feedback-ID: id4a34324:Fastmail
Date: Tue, 7 Feb 2023 22:38:14 -0800
From: Andres Freund <andres@anarazel.de>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: melanieplageman@gmail.com, vignesh21@gmail.com, pryzby@telsasoft.com,
	lukas@fittl.com, alvherre@alvh.no-ip.org, magnus@hagander.net,
	pgsql-hackers@postgresql.org, thomas.munro@gmail.com,
	m.sakrejda@gmail.com
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
Message-ID: <20230208063814.boyh5ibydbfuo2uk@awork3.anarazel.de>
References: <CAAKRu_af0YcV9CXG9M5R06tYRUzk13+H7qOsU6etmS4zE7RAUQ@mail.gmail.com>
 <20230124.172203.448918351330829200.horikyota.ntt@gmail.com>
 <20230124223512.xqp7tjgqvzqokaxq@awork3.anarazel.de>
 <20230125.165617.724951472557617342.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230125.165617.724951472557617342.horikyota.ntt@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/20230208063814.boyh5ibydbfuo2uk%40awork3.anarazel.de>
Precedence: bulk

Hi,

I did another read through the series. I do have some minor changes, but
they're minor. I think this is ready for commit. I plan to start pushing
tomorrow.

The changes I made are:
- the tablespace test changes didn't quite work in isolation / needed a bit of
  polishing
- moved the tablespace changes to later in the series
- split the tests out of the commit adding the view into its own commit
- minor code formatting things (e.g. didn't like nested for()s without {})


On 2023-01-25 16:56:17 +0900, Kyotaro Horiguchi wrote:
> At Tue, 24 Jan 2023 14:35:12 -0800, Andres Freund <andres@anarazel.de> wrote in
> > > +	write_chunk_s(fpout, &pgStatLocal.snapshot.io);
> > > +	if (!read_chunk_s(fpin, &shmem->io.stats))
> > >
> > > The names of the functions hardly make sense alone to me. How about
> > > write_struct()/read_struct()?  (I personally prefer to use
> > > write_chunk() directly..)
> >
> > That's not related to this patch - there's several existing callers for
> > it. And write_struct wouldn't be better imo, because it's not just for
> > structs.
>
> Hmm.  Then what the "_s" stands for?

Size. It's a macro that just forwards to read_chunk()/write_chunk().


> > > > +        Number of read operations in units of <varname>op_bytes</varname>.
> > >
> > > I may be the only one who see the name as umbiguous between "total
> > > number of handled bytes" and "bytes hadled at an operation". Can't it
> > > be op_blocksize or just block_size?
> > >
> > > +       b.io_object,
> > > +       b.io_context,
> >
> > No, block wouldn't be helpful - we'd like to use this for something that isn't
> > uniform blocks.
>
> What does the field show in that case?  The mean of operation size? Or
> one row per opration size?  If the former, the name looks somewhat
> wrong. If the latter, block_size seems making sense.

1, so that it's clear that the rest are in bytes.

Greetings,

Andres Freund