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

From: Melanie Plageman <melanieplageman(at)gmail(dot)com>
To: Andres Freund <andres(at)anarazel(dot)de>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, Magnus Hagander <magnus(at)hagander(dot)net>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org>, Lukas Fittl <lukas(at)fittl(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
Subject: Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
Date: 2022-08-22 17:15:18
Message-ID: CAAKRu_YuFg7C_nr0BBg90w7KSnC1Y=-EXi61J9bzy0HdEuVP3w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

v28 attached.

I've added the new structs I added to typedefs.list.

I've split the commit which adds all of the logic to track
IO operation statistics into two commits -- one which includes all of
the code to count IOOps for IOContexts locally in a backend and a second
which includes all of the code to accumulate and manage these with the
cumulative stats system.

A few notes about the commit which adds local IO Operation stats:

- There is a comment above pgstat_io_op_stats_collected() which mentions
the cumulative stats system even though this commit doesn't engage the
cumulative stats system. I wasn't sure if it was more or less
confusing to have two different versions of this comment.

- should pgstat_count_io_op() take BackendType as a parameter instead of
using MyBackendType internally?

- pgstat_count_io_op() Assert()s that the passed-in IOOp and IOContext
are valid for this BackendType, but it doesn't check that all of the
pending stats which should be zero are zero. I thought this was okay
because if I did add that zero-check, it would be added to
pgstat_count_ioop() as well, and we already Assert() there that we can
count the op. Thus, it doesn't seem like checking that the stats are
zero would add any additional regression protection.

- I've kept pgstat_io_context_desc() and pgstat_io_op_desc() in the
commit which adds those types (the local stats commit), however they
are not used in that commit. I wasn't sure if I should keep them in
that commit or move them to the first commit using them (the commit
adding the new view).

Notes on the commit which accumulates IO Operation stats in shared
memory:

- I've extended the usage of the Assert()s that IO Operation stats that
should be zero are. Previously we only checked the stats validity when
querying the view. Now we check it when flushing pending stats and
when reading the stats file into shared memory.

Note that the three locations with these validity checks (when
flushing pending stats, when reading stats file into shared memory,
and when querying the view) have similar looking code to loop through
and validate the stats. However, the actual action they perform if the
stats are valid is different for each site (adding counters together,
doing a read, setting nulls in a tuple column to true). Also, some of
these instances have other code interspersed in the loops which would
require additional looping if separated from this logic. So it was
difficult to see a way of combining these into a single helper
function.

- I've left pgstat_fetch_backend_io_context_ops() in the shared stats
commit, however it is not used until the commit which adds the view in
pg_stat_get_io(). I wasn't sure which way seemed better.

- Melanie

Attachment Content-Type Size
v28-0003-Track-IO-operation-statistics-locally.patch text/x-patch 22.3 KB
v28-0005-Add-system-view-tracking-IO-ops-per-backend-type.patch text/x-patch 27.3 KB
v28-0002-Remove-unneeded-call-to-pgstat_report_wal.patch text/x-patch 1.1 KB
v28-0001-Add-BackendType-for-standalone-backends.patch text/x-patch 2.7 KB
v28-0004-Aggregate-IO-operation-stats-per-BackendType.patch text/x-patch 21.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2022-08-22 17:36:54 Re: pg_stat_have_stats() returns true for dropped indexes (or for index creation transaction rolled back)
Previous Message Jacob Champion 2022-08-22 16:47:19 Re: CFM Manager