Horton

Thread

Commits

  1. Add information about WAL buffers full to VACUUM/ANALYZE (VERBOSE)

  2. Add information about WAL buffers being full to EXPLAIN (WAL)

  3. pg_stat_statements: Add wal_buffers_full

  4. Move wal_buffers_full from PgStat_PendingWalStats to WalUsage

  5. pg_stat_statements: Add columns to track parallel worker activity

  1. Move wal_buffers_full to WalUsage (and report it in pgss/explain)

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-06T10:27:17Z

    Hi hackers,

While working on per-backend WAL statistics ([1]) it has been discussed to
move wal_buffers_full to WalUsage (It's currently tracked in PgStat_PendingWalStats).

There is some benefits doing so:

- Simplifies [1]
- Adds the ability to report it in pg_stat_statements (among wal_records, wal_fpi
and wal_bytes)
- Adds the ability to report it in explain/auto_explain (among wal_records, wal_fpi
and wal_bytes)

I did a bit of archeology to try to understand why it's not already the
case. From what I can see, in commit time order:

1. df3b181499 introduced the WalUsage structure
2. 6b466bf5f2 added the wal usage in pg_stat_statements
3. 33e05f89c5 added the wal usage in EXPLAIN
4. 8d9a935965f added pg_stat_wal (and wal_buffers_full)
5. 01469241b2f added the wal usage in pg_stat_wal 

So, wal_buffers_full has been introduced after the WalUsage structure was
there but I don't see any reason in the emails/threads as to why it's not in the
WalUsage structure.

==== A few thoughts:

About explain:

One could not be 100% sure that the statement being explained is fully responsible
of the wal buffer being full (as it could just be a "victim" of an already almost
full wal buffer). But OTOH that might help to understand why an EXPLAIN analyze
is slower than another one (i.e one generating wal buffer full and the other not).

About pg_stat_statements:

That could also provide valuable information. Say if one can observe that a
statement generates (at least) a WAL buffer full each time it's executed or not.
The later could explain "un-stable" performance for the statement (if WAL buffer
full is not zero).
The former could try to be improved if needed.

=== Patch's structure

The patch series is made of 3 "small" sub-patches:

0001: to move wal_buffers_full to WalUsage
0002: to report wal_buffers_full in pg_stat_statements
0003: to report wal_buffers_full in explain/auto_explain

==== Some remarks:

0002: it doesn't bump the version of pg_stat_statements as the same is done for
this release in commit cf54a2c00254.

0002 and 0003 don't add tests. It looks like there is no existing tests for
wal_buffers_full, reason probably is that it would not be easy to have a stable
and performant test.

[1]: https://www.postgresql.org/message-id/Z6M7/zq4vgE4lch2%40ip-10-97-1-34.eu-west-3.compute.internal

Looking forward to your feedback,

Regards,

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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-11T09:37:37Z

    Hi,

On Thu, Feb 06, 2025 at 10:27:17AM +0000, Bertrand Drouvot wrote:
> === Patch's structure
> 
> The patch series is made of 3 "small" sub-patches:
> 
> 0001: to move wal_buffers_full to WalUsage
> 0002: to report wal_buffers_full in pg_stat_statements
> 0003: to report wal_buffers_full in explain/auto_explain

While at it, adding 0004 to report wal_buffers_full in VACUUM/ANALYZE (VERBOSE).

Regards,

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

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

    Ilia Evdokimov <ilya.evdokimov@tantorlabs.com> — 2025-02-11T12:30:09Z

    Hi,

Thank you for your work!

1. Perhaps In EXPLAIN you forget to check that usage->wal_buffers_full > 0:

if ((usage->wal_records > 0) || (usage->wal_fpi > 0) || 
(usage->wal_bytes > 0))


2. I have a small suggestion for pg_stat_statements: would it make sense 
to move wal_buffers_full next to wal_records, wal_fpi and wal_bytes? 
This way, all WAL-related information would be grouped together.

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-11T14:05:18Z

    Hi,

On Tue, Feb 11, 2025 at 03:30:09PM +0300, Ilia Evdokimov wrote:
> Hi,
> 
> Thank you for your work!

Thanks for the review!

> 1. Perhaps In EXPLAIN you forget to check that usage->wal_buffers_full > 0:
> 
> if ((usage->wal_records > 0) || (usage->wal_fpi > 0) || (usage->wal_bytes >
> 0))

I don't think that's possible to have wal_buffers_full > 0 if the above returns
false. A check is done at appendStringInfo() time so I think that's ok as it is.

> 2. I have a small suggestion for pg_stat_statements: would it make sense to
> move wal_buffers_full next to wal_records, wal_fpi and wal_bytes? This way,
> all WAL-related information would be grouped together.

I think I prefer to add it in "append" order. That way, that does not break
queries that rely on ordinal numbers.

Regards,

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

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

    Michael Paquier <michael@paquier.xyz> — 2025-02-14T06:02:51Z

    On Tue, Feb 11, 2025 at 02:05:18PM +0000, Bertrand Drouvot wrote:
>> 2. I have a small suggestion for pg_stat_statements: would it make sense to
>> move wal_buffers_full next to wal_records, wal_fpi and wal_bytes? This way,
>> all WAL-related information would be grouped together.
> 
> I think I prefer to add it in "append" order. That way, that does not break
> queries that rely on ordinal numbers.

Not sure.  FWIW, it makes sense to me to group them by "family" in
this case, as they would belong to the same instrument structure.
--
Michael

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

    Michael Paquier <michael@paquier.xyz> — 2025-02-14T06:24:22Z

    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.  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.

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.

Anyway, if there are no objections, I am planning to apply this one to
lift this barrier and help with the follow-up work for the backend
stats.  That's the mandatory piece to makes the backend WAL stats
integration easier. 

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.  If other
have comments, feel free.

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.

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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2025-02-14T08:57:49Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2025-02-17T06:18:04Z

    On Fri, Feb 14, 2025 at 03:02:51PM +0900, Michael Paquier wrote:
> Not sure.  FWIW, it makes sense to me to group them by "family" in
> this case, as they would belong to the same instrument structure.

Looked at the full set again, and in the PGSS patch I have finished by
grouping all the WAL fields together, like we do for the rest.  In
EXPLAIN, we have other fields that use spaces rather than underscores,
see "actual time" for example, so I've done the same for buffers full.

Nothing much more to say, so I've applied the set.  Now let's see what
happens for the WAL stats patch at backend level.
--
Michael