Re: per backend WAL statistics

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.pg@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]