Re: per backend WAL statistics

Hi,

Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> 于2025年3月5日周三 21:03写道：

> Hi,
>
> On Wed, Mar 05, 2025 at 05:45:57PM +0800, Xuneng Zhou wrote:
> > Subject: Clarification Needed on WAL Pending Checks in Patchset
> >
> > Hi,
> >
> > Thank you for the patchset. I’ve spent some time learning and reviewing
> it
> > and have 2 comments.
>
> Thanks for looking at it!
>
> > I noticed that in patches prior to patch 6, the function
> > pgstat_backend_wal_have_pending() was implemented as follows:
> >
> > /*
> > * To determine whether any WAL activity has occurred since last time,
> not
> > * only the number of generated WAL records but also the numbers of WAL
> > * writes and syncs need to be checked. Because even transactions that
> > * generate no WAL records can write or sync WAL data when flushing the
> > * data pages.
> > */
> > static bool
> > pgstat_backend_wal_have_pending(void)
> > {
> > PgStat_PendingWalStats pending_wal;
> >
> > pending_wal = PendingBackendStats.pending_wal;
> >
> > return pgWalUsage.wal_records != prevBackendWalUsage.wal_records ||
> > pending_wal.wal_write != 0 || pending_wal.wal_sync != 0;
> > }
> >
> > Starting with patch 7, it seems the implementation was simplified to:
> >
> > /*
> > * To determine whether WAL usage happened.
> > */
> > static bool
> > pgstat_backend_wal_have_pending(void)
> > {
> > return pgWalUsage.wal_records != prevBackendWalUsage.wal_records;
> > }
>
> >That's right. This is due to 2421e9a51d2 that removed
> PgStat_PendingWalStats.
>
> > Meanwhile, the cluster-level check in the function
> > pgstat_wal_have_pending_cb() still performs the additional checks:
> >
> > bool
> > pgstat_wal_have_pending_cb(void)
> > {
> > return pgWalUsage.wal_records != prevWalUsage.wal_records ||
> > PendingWalStats.wal_write != 0 ||
> > PendingWalStats.wal_sync != 0;
> > }
>
> Not since 2421e9a51d2. It looks like that you are looking at code prior to
>> 2421e9a51d2.
>
>
> Yeh, my local version is behind the main branch.
>
> > Another one is on:
> > Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> 于2025年3月3日周一 18:47写道：
> >
> > > Hi,
> > >
> > > On Mon, Mar 03, 2025 at 09:17:30AM +0000, Bertrand Drouvot wrote:
> > > > hmm, that would work as long as PGSTAT_BACKEND_FLUSH_ALL represents
> > > things
> > > > that need to be called from pgstat_report_wal(). But I think that's
> open
> > > > door for issue should be add a new PGSTAT_BACKEND_FLUSH_XXX where
> XXX is
> > > not
> > > > related to pgstat_report_wal() at all. So, I'm tempted to keep it as
> it
> > > is.
> > >
> > > I just realized that pgstat_flush_backend() is not correct in 0001.
> Indeed
> > > we check:
> > >
> > > "
> > > if (pg_memory_is_all_zeros(&PendingBackendStats,
> > > sizeof(struct PgStat_BackendPending)))
> > > return false;
> > > "
> > >
> > > but the WAL stats are not part of PgStat_BackendPending... So we only
> check
> > > for IO pending stats here. I'm not sure WAL stats could be non empty
> if IO
> > > stats are but the attached now also takes care of pending WAL stats
> here.
> >
> > I've noticed that here's a function for checking if there are any backend
> > stats waiting for flush:
> > /*
> > * Check if there are any backend stats waiting for flush.
> > */
> > bool
> > pgstat_backend_have_pending_cb(void)
> > {
> > return (!pg_memory_is_all_zeros(&PendingBackendStats,
> > sizeof(struct PgStat_BackendPending)));
> > }
>
> That's right.
>>
>> The reason I did not add the extra check there is because I have in mind
>> to
>> replace the pg_memory_is_all_zeros() checks by a new global variable and
>> also replace
>> the checks in pgstat_flush_backend() by a call to
>> pgstat_backend_have_pending_cb()
>> (see 0002 in [1]). It means that all of that would be perfectly clean if
>> 0002 in [1] goes in.
>>
>> But yeah, if 0002 in [1] does not go in, then your concern is valid, so
>> adding
>> the extra check in the attached.
>>
>> Thanks for the review!
>>
>> [1]:
>> https://www.postgresql.org/message-id/Z8WYf1jyy4MwOveQ%40ip-10-97-1-34.eu-west-3.compute.internal
>>
>> Regards,
>
>

> That makes more sense, I'll take a look at thread 1. Separating patches
> helps clarify their purpose, but it may also fragment the overall
> perspective:) Thanks a lot for your explaination!
>

> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>