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

v14 attached.

On Tue, Oct 19, 2021 at 3:29 PM Andres Freund <andres@anarazel.de> wrote:
>
>
> > > 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...
>

done [1]. also, I dropped that commit from this patchset.

>
> > > > @@ -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 :(.
>

I've left what I originally had, then.

>
>
> > > > +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?
>

done but wasn't sure about PGDLLIMPORT

>
> > > 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?
>

done

-- melanie

[1] https://www.postgresql.org/message-id/flat/CAAKRu_azyd1Z3W_r7Ou4sorTjRCs%2BPxeHw1CWJeXKofkE6TuZg%40mail.gmail.com