Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)

Hi,

On 2021-10-11 16:48:01 -0400, Melanie Plageman wrote:
> On Fri, Oct 8, 2021 at 1:56 PM Andres Freund <andres@anarazel.de> wrote:
> > On 2021-10-01 16:05:31 -0400, Melanie Plageman wrote:
> > > diff --git a/src/backend/utils/init/postinit.c b/src/backend/utils/init/postinit.c
> > > index 78bc64671e..fba5864172 100644
> > > --- a/src/backend/utils/init/postinit.c
> > > +++ b/src/backend/utils/init/postinit.c
> > > @@ -670,8 +670,7 @@ InitPostgres(const char *in_dbname, Oid dboid, const char *username,
> > >       EnablePortalManager();
> > >
> > >       /* Initialize status reporting */
> > > -     if (!bootstrap)
> > > -             pgstat_beinit();
> > > +     pgstat_beinit();
> > >
> > >       /*
> > >        * Load relcache entries for the shared system catalogs.  This must create
> > > --
> > > 2.27.0
> > >
> >
> > I think it's good to remove more and more of these !bootstrap cases - they
> > really make it harder to understand the state of the system at various
> > points. Optimizing for the rarely executed bootstrap mode at the cost of
> > checks in very common codepaths...
>
> What scope do you suggest for this patch set? A single patch which does
> this in more locations (remove !bootstrap) or should I remove this patch
> from the patchset?

I think the scope is fine as-is.


> > Is pgstattuple the best place for this helper? It's not really pgstatfuncs
> > specific...
> >
> > It also looks vaguely familiar - I wonder if we have a helper roughly like
> > this somewhere else already...
> >
>
> I don't see a function which is specifically a utility to make a
> tuplestore. Looking at the callers of tuplestore_begin_heap(), I notice
> very similar code to the function I added in pg_tablespace_databases()
> in utils/adt/misc.c, pg_stop_backup_v2() in xlogfuncs.c,
> pg_event_trigger_dropped_objects() and pg_event_trigger_ddl_commands in
> event_tigger.c, pg_available_extensions in extension.c, etc.
>
> Do you think it makes sense to refactor this code out of all of these
> places?

Yes, I think it'd make sense. We have about 40 copies of this stuff, which is
fairly ridiculous.


> If so, where would such a utility function belong?

Not quite sure. src/backend/utils/fmgr/funcapi.c maybe? I suggest starting a
separate thread about that...


> > > @@ -2999,6 +3036,14 @@ pgstat_shutdown_hook(int code, Datum arg)
> > >  {
> > >       Assert(!pgstat_is_shutdown);
> > >
> > > +     /*
> > > +      * Only need to send stats on IO Ops for IO Paths when a process exits, as
> > > +      * pg_stat_get_buffers() will read from live backends' PgBackendStatus and
> > > +      * then sum this with totals from exited backends persisted by the stats
> > > +      * collector.
> > > +      */
> > > +     pgstat_send_buffers();
> > > +
> > >       /*
> > >        * If we got as far as discovering our own database ID, we can report what
> > >        * we did to the collector.  Otherwise, we'd be sending an invalid
> > > @@ -3092,6 +3137,30 @@ pgstat_send(void *msg, int len)
> > >  #endif
> > >  }
> >
> > I think it might be nicer to move pgstat_beshutdown_hook() to be a
> > before_shmem_exit(), and do this in there.
> >
>
> Not really sure the correct way to do this. A cursory attempt to do so
> failed because ShutdownXLOG() is also registered as a
> before_shmem_exit() and ends up being called after
> pgstat_beshutdown_hook(). pgstat_beshutdown_hook() zeroes out
> PgBackendStatus, ShutdownXLOG() initiates a checkpoint, and during a
> checkpoint, the checkpointer increments IO op counter for writes in its
> PgBackendStatus.

I think we'll really need to do a proper redesign of the shutdown callback
mechanism :(.



> > > +static void
> > > +pgstat_recv_io_path_ops(PgStat_MsgIOPathOps *msg, int len)
> > > +{
> > > +     int                     io_path;
> > > +     PgStatIOOps *src_io_path_ops = msg->iop.io_path_ops;
> > > +     PgStatIOOps *dest_io_path_ops =
> > > +     globalStats.buffers.ops[msg->backend_type].io_path_ops;
> > > +
> > > +     for (io_path = 0; io_path < IOPATH_NUM_TYPES; io_path++)
> > > +     {
> > > +             PgStatIOOps *src = &src_io_path_ops[io_path];
> > > +             PgStatIOOps *dest = &dest_io_path_ops[io_path];
> > > +
> > > +             dest->allocs += src->allocs;
> > > +             dest->extends += src->extends;
> > > +             dest->fsyncs += src->fsyncs;
> > > +             dest->writes += src->writes;
> > > +     }
> > > +}
> >
> > Could this, with a bit of finessing, use pgstat_add_io_path_ops()?
> >
>
> I didn't really see a good way to do this -- given that
> pgstat_add_io_path_ops() adds IOOps members to PgStatIOOps members --
> which requires a pg_atomic_read_u64() and pgstat_recv_io_path_ops adds
> PgStatIOOps to PgStatIOOps which doesn't require pg_atomic_read_u64().
> Maybe I could pass a flag which, based on the type, either does or
> doesn't use pg_atomic_read_u64 to access the value? But that seems worse
> to me.

Yea, you're probably right, that's worse.


> > > +PgBackendStatus *
> > > +pgstat_fetch_backend_statuses(void)
> > > +{
> > > +     return BackendStatusArray;
> > > +}
> >
> > Hm, not sure this adds much?
>
> Is there a better way to access the whole BackendStatusArray from within
> pgstatfuncs.c?

Export the variable itself?


> > IIRC Thomas Munro had a patch adding a nonatomic_add or such
> > somewhere. Perhaps in the recovery readahead thread? Might be worth using
> > instead?
> >
>
> I've added Thomas' function in a separate commit. I looked for a better
> place to add it (I was thinking somewhere in src/backend/utils/misc) but
> couldn't find anywhere that made sense.

I think it should just live in atomics.h?


> I also added pgstat_inc_ioop() calls to callers of smgrwrite() flushing local
> buffers. I don't know if that is desirable or not in this patch. They could be
> removed if wrappers for smgrwrite() go in and pgstat_inc_ioop() can be called
> from within those wrappers.

Makes sense to me to to have it here.

Greetings,

Andres Freund