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

On Wed, Dec 01, 2021 at 04:59:44PM -0500, Melanie Plageman wrote:
> 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:

I saw that after I made my suggestion.  Sorry for the noise.
Both ways already exist in postgres and seem to be acceptable.

> > There's extraneous blank lines in these functions:
> > +pgstat_recv_resetsharedcounter
> I didn't see one here

=> The extra blank line is after the RESET_BUFFERS memset.

> +        * Reset the global, bgwriter and checkpointer statistics for the
> +        * cluster.

The first comma in this comment was introduced in 1bc8e7b09, and seems to be
extraneous, since bgwriter and checkpointer are both global.  With the comma,
it looks like it should be memsetting 3 things.

> +               /* Don't count dead backends. They should already be counted */

Maybe this comment should say ".. they'll be added below"

> +                       row[COLUMN_BACKEND_TYPE] = backend_type_desc;
> +                       row[COLUMN_IO_PATH] = CStringGetTextDatum(GetIOPathDesc(io_path));
> +                       row[COLUMN_ALLOCS] += io_ops->allocs - resets->allocs;
> +                       row[COLUMN_EXTENDS] += io_ops->extends - resets->extends;
> +                       row[COLUMN_FSYNCS] += io_ops->fsyncs - resets->fsyncs;
> +                       row[COLUMN_WRITES] += io_ops->writes - resets->writes;
> +                       row[COLUMN_RESET_TIME] = reset_time;

It'd be clearer if RESET_TIME were set adjacent to BACKEND_TYPE and IO_PATH.

> > Rather than memset(), you could initialize msg like this.
> > PgStat_MsgIOPathOps msg = {0};
>
> 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]
> 
> 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.

I think gcc is suggesting to write something like {{0}}, and I think the online
commentary you found is saying that the warning is a false positive.
So I think you should ignore my suggestion - it's not worth the bother.

This message needs to be updated:
    errhint("Target must be \"archiver\", \"bgwriter\", or \"wal\".")))

When I query the view, I see reset times as: 1999-12-31 18:00:00-06.
I guess it should be initialized like this one:
    globalStats.bgwriter.stat_reset_timestamp = ts

The cfbot shows failures now (I thought it was passing with the previous patch,
but I suppose I'm wrong.)

It looks like running recovery during single user mode hits this assertion.
TRAP: FailedAssertion("beentry", File: "../../../../src/include/utils/backend_status.h", Line: 359, PID: 3499)

-- 
Justin