Date: Thu, 12 Jan 2023 23:23:05 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Lukas Fittl <lukas@fittl.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Magnus Hagander <magnus@hagander.net>, pgsql-hackers@postgresql.org,
	Thomas Munro <thomas.munro@gmail.com>,
	Maciek Sakrejda <m.sakrejda@gmail.com>,
	vignesh C <vignesh21@gmail.com>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
Message-ID: <20230113052305.GW9837@telsasoft.com>
References: <CAAKRu_bqPZQ3KQ2Hf6k6v0jATZXA0NSLLurjEYd1HcEznv=E2Q@mail.gmail.com>
 <CAP53PkyL5+h0NfKsY-XQ7fq9ha+37G5gd5vhZYtb6QQyJnvHdg@mail.gmail.com>
 <CAAKRu_ZiLuEPANqsHqqRPbgt4BTmgMqtPpyJJaTQxLs818tvKg@mail.gmail.com>
 <20221228235613.ep47flalmnkqgnu7@awork3.anarazel.de>
 <CAAKRu_YLnLB6fQjqs50wo-K3rt9KoufzXCrJqkz45nd7AvJOug@mail.gmail.com>
 <CAAKRu_aNSet=AoDpGkT6dW7f8UQAPP+HQqVP2f0OrE-Mubi_NQ@mail.gmail.com>
 <CAAKRu_b3rSOr33A6HGKN747QxpB0UkgGfjhhbn9eaEcp24HyhQ@mail.gmail.com>
 <CAAKRu_acW=dAre8xOeEoNT5Fk0d8obCv5+Lf3JcinJ1u24fLog@mail.gmail.com>
 <20230111215850.GO9837@telsasoft.com>
 <CAAKRu_bTjvHSd5tWWQkW8SWY-5frVp_vOJpFD+-V7jJG1Rf_Dw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAKRu_bTjvHSd5tWWQkW8SWY-5frVp_vOJpFD+-V7jJG1Rf_Dw@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20230113052305.GW9837%40telsasoft.com>
Precedence: bulk

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

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

> > 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..".

+        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<).

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

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

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

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

-- 
Justin