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

Attached is v47.

On Fri, Jan 13, 2023 at 12:23 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Jan 12, 2023 at 09:19:36PM -0500, Melanie Plageman wrote:
> > On Wed, Jan 11, 2023 at 4:58 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > >
> > > > Subject: [PATCH v45 4/5] Add system view tracking IO ops per backend type
> > >
> > > The patch can/will fail with:
> > >
> > > CREATE TABLESPACE test_io_shared_stats_tblspc LOCATION '';
> > > +WARNING:  tablespaces created by regression test cases should have names starting with "regress_"
> > >
> > > CREATE TABLESPACE test_stats LOCATION '';
> > > +WARNING:  tablespaces created by regression test cases should have names starting with "regress_"
> > >
> > > (I already sent patches to address the omission in cirrus.yml)
> >
> > Thanks. I've fixed this
> > I make a tablespace in amcheck -- are there recommendations for naming
> > tablespaces in contrib also?
>
> That's the test_stats one I mentioned.
>
> Check with -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

Thanks. I have now changed both tablespace names and checked using that
macro.

> > > > +          <literal>bulkread</literal>: Qualifying large read I/O operations
> > > > +          done outside of shared buffers, for example, a sequential scan of a
> > > > +          large table.
> > >
> > > I don't think it's correct to say that it's "outside of" shared-buffers.
> >
> > I suppose "outside of" gives the wrong idea. But I need to make clear
> > that this I/O is to and from buffers which are not a part of shared
> > buffers right now -- they may still be accessible from the same data
> > structures which access shared buffers but they are currently being used
> > in a different way.
>
> This would be a good place to link to a description of the ringbuffer,
> if we had one.

Indeed.

> > > s/Qualifying/Certain/
> >
> > I feel like qualifying is more specific than certain, but I would be open
> > to changing it if there was a specific reason you don't like it.
>
> I suggested to change it because at first I started to interpret it as
> "The act of qualifying large I/O ops .." rather than "Large I/O ops that
> qualify..".

I have changed it to "certain".

> +        Number of read operations of <varname>op_bytes</varname> size.
>
> This is still a bit too easy to misinterpret as being in units of bytes.
> I suggest: Number of read operations (which are each of the size
> specified in >op_bytes<).

I have changed this.

> + in order to add the shared buffer to a separate size-limited ring buffer
>
> separate comma
>
> + More information on configuring checkpointer can be found in Section 30.5.
>
> *the* checkpointer (as in the following paragraph)

above items changed.

> +   <varname>backend_type</varname> <literal>checkpointer</literal> and
> +   <varname>io_object</varname> <literal>temp relation</literal>.
> +  </para>
>
> I still think it's a bit hard to understand the <varname>s adjacent to
> <literal>s.

I agree it isn't great -- is there a different XML tag you suggest
instead of literal?

> + Some backend_types
> + in some io_contexts
> + on some io_objects
> + in certain io_contexts
> + on certain io_objects
>
> Maybe these should not use underscores:  Some backend types never
> perform I/O operations in some I/O contexts and/or on some i/o objects.

I've changed this.

Also, taking another look, I forgot to update the docs' column name
tenses in the last version. That is now done.

> + for (BackendType bktype = B_INVALID; bktype < BACKEND_NUM_TYPES; bktype++)
> + for (IOContext io_context = IOCONTEXT_BULKREAD; io_context < IOCONTEXT_NUM_TYPES; io_context++)
> + for (IOObject io_obj = IOOBJECT_RELATION; io_obj < IOOBJECT_NUM_TYPES; io_obj++)
> + for (IOOp io_op = IOOP_EVICT; io_op < IOOP_NUM_TYPES; io_op++)
>
> These look a bit fragile due to starting at some hardcoded "first"
> value.  In other places you use symbols "FIRST" symbols:
>
> +       for (IOContext io_context = IOCONTEXT_FIRST; io_context < IOCONTEXT_NUM_TYPES; io_context++)
> +               for (IOObject io_object = IOOBJECT_FIRST; io_object < IOOBJECT_NUM_TYPES; io_object++)
> +                       for (IOOp io_op = IOOP_FIRST; io_op < IOOP_NUM_TYPES; io_op++)
>
> I think that's marginally better, but I think having to define both
> FIRST and NUM is excessive and doesn't make it less fragile.  Not sure
> what anyone else will say, but I'd prefer if it started at "0".

Thanks for catching the discrepancy in pg_stat_get_io(). I have changed
those instances to use _FIRST.

I think that having the loop start from the first enum value (except
when that value is something special like _INVALID like with
BackendType) is confusing. I agree that having multiple macros to allow
iteration through all enum values introduces some fragility. I'm not
sure about using the number 0 with the enum as the loop variable
data type. Is that a common pattern?

In this version, I have updated the loops in pg_stat_get_io() to use
_FIRST.

> Thanks for working on this - I'm looking forward to updating my rrdtool
> script for this soon.  It'll be nice to finally distinguish huge number
> of "backend ringbuffer writes during ALTER" from other backend writes.
> Currently, that makes it look like something is terribly wrong.

Cool! I'm glad to know you will use it.

- Melanie