Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?) - Mailing list pgsql-hackers

Thanks for the review!

On Wed, Nov 24, 2021 at 8:16 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> You wrote beentry++ at the start of two loops, but I think that's wrong; it
> should be at the end, as in the rest of the file (or as a loop increment).
> BackendStatusArray[0] is actually used (even though its backend has
> backendId==1, not 0).  "MyBEEntry = &BackendStatusArray[MyBackendId - 1];"

I've fixed this in v16 which I will attach to the next email in the thread.

> You could put *_NUM_TYPES as the last value in these enums, like
> NUM_AUXPROCTYPES, NUM_PMSIGNALS, and NUM_PROCSIGNALS:
>
> +#define IOOP_NUM_TYPES (IOOP_WRITE + 1)
> +#define IOPATH_NUM_TYPES (IOPATH_STRATEGY + 1)
> +#define BACKEND_NUM_TYPES (B_LOGGER + 1)

I originally had it as you describe, but based on this feedback upthread
from Álvaro Herrera:

> (It's weird to have enum values that are there just to indicate what's
> the maximum value.  I think that sort of thing is better done by having
> a "#define LAST_THING" that takes the last valid value from the enum.
> That would free you from having to handle the last value in switch
> blocks, for example.  LAST_OCLASS in dependency.h is a precedent on this.)

So, I changed it to use macros.

> There's extraneous blank lines in these functions:
>
> +pgstat_sum_io_path_ops

Fixed

> +pgstat_report_live_backend_io_path_ops

I didn't see one here

> +pgstat_recv_resetsharedcounter

I didn't see one here

> +GetIOPathDesc

Fixed

> +StrategyRejectBuffer

Fixed

> This function is doubly-indented:
>
> +pgstat_send_buffers_reset

Fixed. Thanks for catching this.
I also ran pgindent and manually picked a few of the formatting fixes
that were relevant to code I added.

>
> As support for C99 is now required by postgres, variables can be declared as
> part of various loops.
>
> +       int                     io_path;
> +       for (io_path = 0; io_path < IOPATH_NUM_TYPES; io_path++)

Fixed this and all other occurrences in my code.

> Rather than memset(), you could initialize msg like this.
> PgStat_MsgIOPathOps msg = {0};
>
> +pgstat_send_buffers(void)
> +{
> +       PgStat_MsgIOPathOps msg;
> +
> +       PgBackendStatus *beentry = MyBEEntry;
> +
> +       if (!beentry)
> +               return;
> +
> +       memset(&msg, 0, sizeof(msg));
>

though changing the initialization to universal zero initialization
seems to be the correct way, I do get this compiler warning when I make
the change

pgstat.c:3212:29: warning: suggest braces around initialization of
subobject [-Wmissing-braces]
        PgStat_MsgIOPathOps msg = {0};
                                   ^
                                   {}
I have seem some comments online that say that this is a spurious
warning present with some versions of both gcc and clang when using
-Wmissing-braces to compile code with universal zero initialization, but
I'm not sure what I should do.

v16 attached in next message

- Melanie