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(at)anarazel(dot)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