Feedback-ID: id4a34324:Fastmail
Date: Thu, 20 Oct 2022 10:31:01 -0700
From: Andres Freund <andres@anarazel.de>
To: Melanie Plageman <melanieplageman@gmail.com>
Cc: Maciek Sakrejda <m.sakrejda@gmail.com>, Lukas Fittl <lukas@fittl.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	Magnus Hagander <magnus@hagander.net>,
	Pg Hackers <pgsql-hackers@postgresql.org>,
	Thomas Munro <thomas.munro@gmail.com>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and
 more?)
Message-ID: <20221020173101.32d2is2ldsdym7jl@awork3.anarazel.de>
References: 
 <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>
 <CAAKRu_Yu7KKKA8YRsQP1wuU36Ypm7BkYu2yDoc67kdR07zG_Gw@mail.gmail.com>
 <CAAKRu_a14KddNeYd7Rcoocn7g2ZE6sQ2gOjuadHCmjPEqt1Atg@mail.gmail.com>
 <CAOtHd0Bqn68FdHA8L7g59LMDfYmZ+sh-L8=tHrKQDuhPyFV=8g@mail.gmail.com>
 <CAAKRu_a_7JDexgYGFGeNiFfjBBkhwcPwjnkesNnTuF6hFgUY5A@mail.gmail.com>
 <CAOtHd0Aj-F1ogXiEWE4wV5U8A8a-mxS=hYwx9B3fsg57hG2zWg@mail.gmail.com>
 <CAAKRu_agmNMyMbfffGegbgFpUUTL=k3U64n_v0gLQGW5DeDkhg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAAKRu_agmNMyMbfffGegbgFpUUTL=k3U64n_v0gLQGW5DeDkhg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221020173101.32d2is2ldsdym7jl%40awork3.anarazel.de>
Precedence: bulk

Hi,

- we shouldn't do pgstat_count_io_op() while the buffer header lock is held,
  if possible.

  I wonder if we should add a "source" output argument to
  StrategyGetBuffer(). Then nearly all the counting can happen in
  BufferAlloc().

- "repossession" is a very unintuitive name for me. If we want something like
  it, can't we just name it reuse_failed or such?

- Wonder if the column names should be reads, writes, extends, etc instead of
  the current naming pattern

- Is it actually correct to count evictions in StrategyGetBuffer()? What if we
  then decide to not use that buffer in BufferAlloc()? Yes, that'll be counted
  via rejected, but that still leaves the eviction count to be "misleading"?


On 2022-10-19 15:26:51 -0400, Melanie Plageman wrote:
> I have made some major changes in this area to make the columns more
> useful. I have renamed and split "clocksweeps". It is now "evicted" and
> "freelist acquired". This makes it clear when a block must be evicted
> from a shared buffer must be and may help to identify misconfiguration
> of shared buffers.

I'm not sure freelist acquired is really that useful? If we don't add it, we
should however definitely not count buffers from the freelist as evictions.


> There is some nuance here that I tried to make clear in the docs.
> "freelist acquired" in a shared context is straightforward.
> "freelist acquired" in a strategy context is counted when a shared
> buffer is added to the strategy ring (not when it is reused).

Not sure what the second half here means - why would a buffer that's not from
the freelist ever be counted as being from the freelist?


> "freelist_acquired" is confusing for local buffers but I wanted to
> distinguish between reuse/eviction of local buffers and initial
> allocation. "freelist_acquired" seemed more fitting because there is a
> clocksweep to find a local buffer and if it hasn't been allocated yet it
> is allocated in a place similar to where shared buffers acquire a buffer
> from the freelist. If I didn't count it here, I would need to make a new
> column only for local buffers called "allocated" or something like that.

I think you're making this too granular. We need to have more detail than
today. But we don't necessarily need to catch every nuance.


> I chose not to call "evicted" "sb_evicted"
> because then we would need a separate "local_evicted". I could instead
> make "local_evicted", "sb_evicted", and rename "reused" to
> "strat_evicted". If I did that we would end up with separate columns for
> every IO Context describing behavior when a buffer is initially acquired
> vs when it is reused.
>
> It would look something like this:
>
> shared buffers:
>     initial: freelist_acquired
>     reused: sb_evicted
>
> local buffers:
>     initial: allocated
>     reused: local_evicted
>
> strategy buffers:
>     initial: sb_evicted | freelist_acquired
>     reused: strat_evicted
>     replaced: sb_evicted | freelist_acquired
>
> This seems not too bad at first, but if you consider that later we will
> add other kinds of IO -- eg WAL IO or temporary file IO, we won't be
> able to use these existing columns and will need to add even more
> columns describing the exact behavior in those cases.

I think it's clearly not the right direction.


> I have also added the columns "repossessed" and "rejected". "rejected"
> is when a bulkread rejects a strategy buffer because it is dirty and
> requires flush. Seeing a lot of rejections could indicate you need to
> vacuum. "repossessed" is the number of times a strategy buffer was
> pinned or in use by another backend and had to be removed from the
> strategy ring and replaced with a new shared buffer. This gives you some
> indication that there is contention on blocks recently used by a
> strategy.

I don't immediately see a real use case for repossessed. Why isn't it
sufficient to count it as part of rejected?

Greetings,

Andres Freund