Hi,
On Fri, 10 Jan 2025 at 04:47, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Jan 09, 2025 at 03:30:37PM +0000, Bertrand Drouvot wrote:
> > We first use an Assert in is_ioop_tracked_in_bytes() and then we return
> > a value "just" to check another Assert. I wonder if it wouldn't make more sense
> > to get rid of this function and use a macro instead, something like?
> >
> > #define is_ioop_tracked_in_bytes(io_op) \
> > ((io_op) < IOOP_NUM_TYPES && (io_op) >= IOOP_EXTEND)
>
> Indeed. Your suggestion to use a macro makes more sense to me because
> is_ioop_tracked_in_bytes() itself has an Assert(), while being itself
> only used in an assertion, as you say. Better to document the
> dependency on the ordering of IOOp, even if that's kind of hard to
> miss.
I did not make it like this because now the
pgstat_is_ioop_tracked_in_bytes macro will return false when io_op >=
IOOP_NUM_TYPES. But I agree that having a macro has more benefits,
also there already is a check for the 'io_op < IOOP_NUM_TYPES' in the
pgstat_count_io_op() function.
>
> > v7-0002:
> >
> > I wonder if it wouldn't make more sense to remove pgstat_count_io_op() first
> > and then implement what currently is in v7-0001. What v7-0002 is removing is
> > not produced by v7-0001.
>
> This kind of cleanup should happen first, and it simplifies a bit the
> reading of v7-0001 as we would just append a new argument to the
> count_io_op() routine for the number of bytes. I was looking at that
> and chose to stick with count_io_op() rather than count_io_op_n() as
> we would have only one routine.
>
> pgstat_count_io_op_time(IOOBJECT_RELATION, io_context, IOOP_EXTEND,
> - io_start, extend_by);
> + io_start, 1, extend_by * BLCKSZ);
> [...]
> pgstat_count_io_op_time(IOOBJECT_TEMP_RELATION, IOCONTEXT_NORMAL, IOOP_EXTEND,
> - io_start, extend_by);
> + io_start, 1, extend_by * BLCKSZ);
>
> Something worth mentioning. I had a small doubt about this one for
> temp and persistent relations, as it changes the extend count.
> Anyway, I think that this is better: we are only doing one extend
> batch, worth (extend_by * BLCKSZ).
I agree with you.
>
> Two times my fault today that the main patch does not apply anymore
> (three times at least in total), so I have rebased it myself, and did
> an extra review while on it, switching the code to use a macro. That
> seems OK here. Please let me know if you have more comments.
No worries, thank you for all of these!
--
Regards,
Nazir Bilal Yavuz
Microsoft
