Re: Move wal_buffers_full to WalUsage (and report it in pgss/explain)

Hi,

On Fri, Feb 14, 2025 at 03:24:22PM +0900, Michael Paquier wrote:
> On Tue, Feb 11, 2025 at 09:37:37AM +0000, Bertrand Drouvot wrote:
> > While at it, adding 0004 to report wal_buffers_full in VACUUM/ANALYZE (VERBOSE).
>
> Thanks for summarizing the history behind WalUsage and
> wal_buffers_full.

Thanks for looking at it.

> FWIW, 0001 that moves wal_buffers_full from
> PgStat_PendingWalStats to WalUsage is going to make our lives much
> easier for your pending patch to adds backend statistics for WAL. WAL
> write/sync numbers/times will be covered in the backend stats by
> pg_stat_io, allowing us to remove entirely the dependency to
> PgStat_PendingWalStats.

Yup.

> I have been wondering for a bit if the comment at the top of WalUsage
> should be updated, but the current description fits as well with the
> follow-up patch series.

Agree that the comment is "still" fine with the extra struct member.

> 0002, 0003 and 0004 are straight-forward follow-ups. It's IMO one of
> these things where extra data is cheap to have access to, and can be
> useful to be aware when distributed across multiple contexts like
> queries, plans or even EXPLAIN. So no real objections here.

Yeah and that makes the comment at the top of WalUsage accurate ;-)

"
and is displayed by EXPLAIN command, pg_stat_statements extension, etc
"

> show_wal_usage() should have its check on (usage->wal_buffers_full) in
> explain.c, as Ilia has mentioned. It's true that you would not get a
> (wal_buffers_full > 0) if at least the condition on wal_bytes is not
> satisfied, but the addition makes sense on consistency grounds, at
> least.

I'm thinking the opposite and think that the current check could be simplfied to
"usage->wal_bytes > 0" only. I don't think that wal_records and wal_fpi and
wal_buffers_full can be = 0 if wal_bytes > 0. I don't think that's worth a dedicated
thread and could be done in passing instead of adding the check on wal_buffers_full.

Done that way in the attached (added a comment in the commit message though).

> Agreed about the attribute ordering in pgss with everything associated
> to WalUsage grouped together, btw.

Ok, you have the majority then and looking at the ordering of the "api_version >="
checks in pg_stat_statements_internal() it looks like it's not the first time
we'd break queries relying on ordinal numbers. Done that way in the attached.
Note that the attached also changes the ordering in the Counters struct (to be
consistent with what 5147ab1dd34a did for example).

Regards,

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