Hi, On Fri, 10 Jan 2025 at 04:47, Michael Paquier <mich...@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
