Re: per backend WAL statistics

On Sat, Mar 08, 2025 at 07:53:04AM +0000, Bertrand Drouvot wrote:
> That would not be an issue should we only access the struct
> fields in the code, but that's not the case as we're making use of
> pg_memory_is_all_zeros() on it.

It does not hurt to keep it as it is, honestly.

I've reviewed the last patch of the series, and noticed a couple of
inconsistent comments across it, and some indentation issue.

@@ -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;

I have one issue with pgstat_flush_backend() and the early exit check
done here. If for example we use FLUSH_WAL but there is some IO data
pending, we would lock the stats entry for nothing. We could also
return true even if there is no pending WAL data if the lock could not
be taken, which would be incorrect because there was no data to flush
to begin with. I think that this should be adjusted so as we limit
the entry lock depending on the flags given in input, like in the
attached.

Thoughts?
--
Michael