Date: Wed, 11 Jan 2023 15:58:51 -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>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
Message-ID: <20230111215850.GO9837@telsasoft.com>
References: <20220825191527.ki6wggnawlrl53i6@awork3.anarazel.de>
 <CAAKRu_YKc8dp3CjeodR=4FJ30=aw8EQz_98S7a_GJ-ozqHW+qQ@mail.gmail.com>
 <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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAAKRu_acW=dAre8xOeEoNT5Fk0d8obCv5+Lf3JcinJ1u24fLog@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20230111215850.GO9837%40telsasoft.com>
Precedence: bulk

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

1760             :                  errhint("Target must be \"archiver\", \"io\", \"bgwriter\", \"recovery_prefetch\", or \"wal\".")));
=> Do you want to put these in order?

pgstat_get_io_op_name() isn't currently being hit by tests; actually,
it's completely unused.

FlushRelationBuffers() isn't being hit for local buffers.

> +      <entry><structname>pg_stat_io</structname><indexterm><primary>pg_stat_io</primary></indexterm></entry>
> +      <entry>
> +       One row per backend type, context, target object combination showing
> +       cluster-wide I/O statistics.

I suggest: "One row for each combination of of .."

> +   The <structname>pg_stat_io</structname> and
> +   <structname>pg_statio_</structname> set of views are especially useful for
> +   determining the effectiveness of the buffer cache.  When the number of actual
> +   disk reads is much smaller than the number of buffer hits, then the cache is
> +   satisfying most read requests without invoking a kernel call.

I would change this say "Postgres' own buffer cache is satisfying ..."

> However, these
> +   statistics do not give the entire story: due to the way in which
> +   <productname>PostgreSQL</productname> handles disk I/O, data that is not in
> +   the <productname>PostgreSQL</productname> buffer cache might still reside in
> +   the kernel's I/O cache, and might therefore still be fetched without

I suggest to refer to "the kernel's page cache"

> +   The <structname>pg_stat_io</structname> view will contain one row for each
> +   backend type, I/O context, and target I/O object combination showing
> +   cluster-wide I/O statistics. Combinations which do not make sense are
> +   omitted.

"..for each combination of .."

> +          <varname>io_context</varname> for a type of I/O operation. For

"for I/O operations"

> +          <literal>vacuum</literal>: I/O operations done outside of shared
> +          buffers incurred while vacuuming and analyzing permanent relations.

s/incurred/performed/

> +          <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.
s/Qualifying/Certain/

> +          <literal>bulkwrite</literal>: Qualifying large write I/O operations
> +          done outside of shared buffers, such as <command>COPY</command>.

Same

> +        Target object of an I/O operation. Possible values are:
> +       <itemizedlist>
> +        <listitem>
> +         <para>
> +          <literal>relation</literal>: This includes permanent relations.

It says "includes permanent" but what seems to mean is that it
"exclusive of temporary relations".

> +     <row>
> +      <entry role="catalog_table_entry">
> +       <para role="column_definition">
> +        <structfield>read</structfield> <type>bigint</type>
> +       </para>
> +       <para>
> +        Number of read operations in units of <varname>op_bytes</varname>.

This looks too much like it means "bytes".
Should say: "in number of blocks of size >op_bytes<"

But wait - is it the number of read operations "in units of op_bytes"
(which would means this already multiplied by op_bytes, and is in units
of bytes).

Or the "number of read operations" *of* op_bytes chunks ?  Which would
mean this is a "pure" number, and could be multipled by op_bytes to
obtain a size in bytes.

> +        Number of write operations in units of <varname>op_bytes</varname>.

> +        Number of relation extend operations in units of
> +        <varname>op_bytes</varname>.

same

> +        In <varname>io_context</varname> <literal>normal</literal>, this counts
> +        the number of times a block was evicted from a buffer and replaced with
> +        another block. In <varname>io_context</varname>s
> +        <literal>bulkwrite</literal>, <literal>bulkread</literal>, and
> +        <literal>vacuum</literal>, this counts the number of times a block was
> +        evicted from shared buffers in order to add the shared buffer to a
> +        separate size-limited ring buffer.

This never defines what "evicted" means.  Does it mea that a dirty
buffer was written out ?

> +        The number of times an existing buffer in a size-limited ring buffer
> +        outside of shared buffers was reused as part of an I/O operation in the
> +        <literal>bulkread</literal>, <literal>bulkwrite</literal>, or
> +        <literal>vacuum</literal> <varname>io_context</varname>s.

Maybe say "as part of a bulk I/O operation (bulkread, bulkwrite, or
vacuum)."

> +  <para>
> +   <structname>pg_stat_io</structname> can be used to inform database tuning.

> +   For example:
> +   <itemizedlist>
> +    <listitem>
> +     <para>
> +      A high <varname>evicted</varname> count can indicate that shared buffers
> +      should be increased.
> +     </para>
> +    </listitem>
> +    <listitem>
> +     <para>
> +      Client backends rely on the checkpointer to ensure data is persisted to
> +      permanent storage. Large numbers of <varname>files_synced</varname> by
> +      <literal>client backend</literal>s could indicate a misconfiguration of
> +      shared buffers or of checkpointer. More information on checkpointer

of *the* checkpointer

> +      Normally, client backends should be able to rely on auxiliary processes
> +      like the checkpointer and background writer to write out dirty data as

*the* bg writer

> +      much as possible. Large numbers of writes by client backends could
> +      indicate a misconfiguration of shared buffers or of checkpointer. More

*the* ckpointer

Should this link to various docs for checkpointer/bgwriter ?

Maybe the docs for ALTER/COPY/VACUUM/CREATE/etc should be updated to
refer to some central description of ring buffers.  Maybe something
should be included to the appendix.

-- 
Justin