public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Bertrand Drouvot <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Nazir Bilal Yavuz <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Cc: [email protected]
Subject: Re: per backend I/O statistics
Date: Mon, 20 Jan 2025 21:11:16 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <Z44vMD/[email protected]>
References: <[email protected]>
<[email protected]>
<66efowskppsns35v5u2m7k4sdnl7yoz5bo64tdjwq7r5lhplrz@y7dme5xwh2r5>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
<Z44vMD/[email protected]>
On Mon, Jan 20, 2025 at 11:10:40AM +0000, Bertrand Drouvot wrote:
> I think that it would be better to make the distinction based on "local/static"
> vs "dynamic memory" pending stats instead: I did so in v3 attached, using:
>
> .flush_dynamic_cb(): flushes pending entries tracked in dynamic memory
> .flush_static_cb(): flushes pending stats from static/global variable
"static" and "dynamic" feel a bit off when used together. For
"static", the stats are from a static state but they can be pushed to
the dshash, as well, which is in dynamic shared memory.
Hmm. Thinking more about that, I think that we should leave the
existing "flush_pending_cb" alone. For the two others, how about
"have_static_pending_cb" and "flush_static_cb" rather than "fixed"?
It seems important to me to show the relationship between these two.
> Not sure about this one, see above. I mean it is currently Ok but once we'll
> introduce the WAL part then it will not be correct depending of the flag value
> being passed.
>
> So, I did put back the previous logic in place (setting to zero only the stats
> the flush callback is responsible for) in v3 attached.
Hmm. I think that I'm OK with what you are doing here with the next
parts in mind, based on this argument.
> I don't think setting have_backendstats to false in pgstat_flush_backend()
> is correct. I mean, it is correct currently but once we'll add the WAL part it
> won't necessary be correct if the flags != PGSTAT_BACKEND_FLUSH_ALL. So, using a
> pg_memory_is_all_zeros check on PendingBackendStats instead in the attached.
Indeed. I got this part slightly wrong here. What you are doing with
an all-zero check looks simpler in the long run.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../[email protected]/2-signature.asc)
download
view thread (39+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: per backend I/O statistics
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox