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. I'm new to PostgreSQL hacking, so please bear with me
if I make mistakes or say something that seems trivial.
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;
}
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;
}
The difference lies in the removal of the checks for wal_write and wal_sync
from the per-backend function. I assume that this may be due to factoring
out these counters—perhaps because they can now be extracted from
pg_stat_get_backend_io(). However, I’m not entirely sure I grasp the full
rationale behind this change.
Another one is on:
Bertrand Drouvot <bertranddrouvot...@gmail.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.
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
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)));
}
[PGSTAT_KIND_BACKEND] = {
....
.have_static_pending_cb = pgstat_backend_have_pending_cb,
.flush_static_cb = pgstat_backend_flush_cb,
.reset_timestamp_cb = pgstat_backend_reset_timestamp_cb,
},
Should the following modification be applied to the above function as well.
Or should we modify the comment 'any backend stat' if we choose to check
i/o only.
@@ -199,7 +258,8 @@ pgstat_flush_backend(bool nowait, bits32 flags)
return false;
if (pg_memory_is_all_zeros(&PendingBackendStats,
- sizeof(struct PgStat_BackendPending)))
+ sizeof(struct PgStat_BackendPending))
+ && !pgstat_backend_wal_have_pending())
return false;
Best regards,
[Xuneng]
