At Tue, 7 Feb 2023 22:38:14 -0800, Andres Freund <andres@anarazel.de> wrote in
> 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().
I know what the macros do. But, I'm fine with the names as they are
there since before this patch. Sorry for the noise.
> > > > > + 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.
Thanks. Okay, I guess the documentation will be changed as necessary.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center