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 <and...@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
