Subject: Re: [BUG]: the walsender does not update its IO statistics until it exits

From: Michael Paquier <[email-masked]>
Date: Fri, 28 Feb 2025 14:41:34 +0900

On Wed, Feb 26, 2025 at 09:48:50AM +0000, Bertrand Drouvot wrote:
> Yeah I think that makes sense, done that way in the attached.
> 
> Speaking about physical walsender, I moved the test to 001_stream_rep.pl instead
> (would also fail without the fix).

Hmm.  I was doing some more checks with this patch, and on closer look
I am wondering if the location you have chosen for the stats reports
is too aggressive: this requires a LWLock for the WAL sender backend
type taken in exclusive mode, with each step of WalSndLoop() taken
roughly each time a record or a batch of records is sent.  A single
installcheck with a primary/standby setup can lead to up to 50k stats
report calls.

With smaller records, the loop can become hotter, can't it?  Also,
there can be a high number of WAL senders on a single node, and I've
heard of some customers with complex logical decoding deployments with
dozens of logical WAL senders.  Isn't there a risk of having this code
path become a point of contention?  It seems to me that we should
benchmark this change more carefully, perhaps even reduce the
frequency of the report calls.
--
Michael

---

From: Michael Paquier <[email-masked]>
Date: Fri, 28 Feb 2025 17:44:20 +0900

> frequency of the report calls.

One idea here would be to have on a single host one server with a set
of N pg_receivewal processes dumping their WAL segments into a tmpfs,
while a single session generates a bunch of records with a minimal
size using pg_logical_emit_message().  Monitoring the maximum
replication lag with pg_stat_replication and looking at some perf
profiles of the cluster should show how these stats reports affect the
replication setup efficiency. 
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Fri, 28 Feb 2025 10:39:31 +0000

Hi,

> installcheck with a primary/standby setup can lead to up to 50k stats

Yeah, what I can observe (installcheck with a primary/standby):

- extras flush are called about 70K times.

Among those 70K:

- about 575 are going after the "have_iostats" check in pgstat_io_flush_cb()
- about 575 are going after the PendingBackendStats pg_memory_is_all_zeros()
check in pgstat_flush_backend() (makes sense due to the above)
- about 575 are going after the PendingBackendStats.pending_io pg_memory_is_all_zeros()
check in pgstat_flush_backend_entry_io() (makes sense due to the above)

It means that only a very few of them are "really" flushing IO stats.

> frequency of the report calls.

That sounds a good idea to measure the impact of those extra calls and see
if we'd need to mitigate the impacts. I'll collect some data.

Regards,

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

---

From: Michael Paquier <[email-masked]>
Date: Mon, 3 Mar 2025 10:51:19 +0900

> if we'd need to mitigate the impacts. I'll collect some data.

Thanks.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 3 Mar 2025 11:54:39 +0000

Hi,

> > if we'd need to mitigate the impacts. I'll collect some data.

So I did some tests using only one walsender (given the fact that the extra
lock you mentioned above is "only" for this particular backend).

=== Test with pg_receivewal

I was using one pg_receivewal process and did some tests that way:

pgbench -n -c8 -j8 -T60 -f <(echo "SELECT pg_logical_emit_message(true, 'test', repeat('0', 1));";)

I did not measure any noticeable extra lag (I did measure the time it took
for pg_size_pretty(sent_lsn - write_lsn) from pg_stat_replication to be back
to zero).

During the pgbench run a "perf record --call-graph fp -p <walsender_pid>" would
report (perf report -n):

1. pgstat_flush_backend() appears at about 3%
2. pg_memory_is_all_zeros() at about 2.8%
3. pgstat_flush_io() at about 0.4%

So it does not look like what we're adding here can be seen as a primary bottleneck.

That said it looks like that there is room for improvment in pgstat_flush_backend()
and that relying on a "have_iostats" like variable would be better than those
pg_memory_is_all_zeros() calls.

That's done in 0001 attached, by doing so, pgstat_flush_backend() now appears at
about 0.2%.

=== Test with pg_recvlogical

Now it does not look like pg_receivewal had a lot of IO stats to report (looking at
pg_stat_get_backend_io() output for the walsender).

Doing the same test with "pg_recvlogical -d postgres -S logical_slot -f /dev/null  --start"
reports much more IO stats.

What I observe without the "have_iostats" optimization is:

1. I did not measure any noticeable extra lag
2. pgstat_flush_io() at about 5.5% (pgstat_io_flush_cb() at about 5.3%)
3  pgstat_flush_backend() at about 4.8%

and with the "have_iostats" optimization I now see pgstat_flush_backend() at
about 2.51%.

So it does not look like what we're adding here can be seen as a primary bottleneck
but that is probably worth implementing the "have_iostats" optimization attached.

Also, while I did not measure any noticeable extra lag, given the fact that 
pgstat_flush_io() shows at about 5.5% and pgstat_flush_backend() at about 2.5%,
that could still make sense to reduce the frequency of the flush calls, thoughts?

Regards,

>From 226f767f90e82ee128319f43226d4d0056756fa9 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Tue, 25 Feb 2025 10:18:05 +0000
Subject: [PATCH v3 1/2] Flush the IO statistics of active walsenders

The walsender does not flush its IO statistics until it exits.
The issue is there since pg_stat_io has been introduced in a9c70b46dbe.
This commits:

1. ensures it does not wait to exit to flush its IO statistics
2. adds a test for a physical walsender (a logical walsender had the same issue
but the fix is in the same code path)
---
 src/backend/replication/walsender.c   |  7 +++++++
 src/test/recovery/t/001_stream_rep.pl | 14 ++++++++++++++
 2 files changed, 21 insertions(+)
  26.8% src/backend/replication/
  73.1% src/test/recovery/t/

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 446d10c1a7d..9ddf111af25 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -90,6 +90,7 @@
 #include "utils/guc.h"
 #include "utils/memutils.h"
 #include "utils/pg_lsn.h"
+#include "utils/pgstat_internal.h"
 #include "utils/ps_status.h"
 #include "utils/timeout.h"
 #include "utils/timestamp.h"
@@ -2829,6 +2830,12 @@ WalSndLoop(WalSndSendDataCallback send_data)
 		/* Send keepalive if the time has come */
 		WalSndKeepaliveIfNecessary();
 
+		/*
+		 * Report IO statistics
+		 */
+		pgstat_flush_io(false);
+		(void) pgstat_flush_backend(false, PGSTAT_BACKEND_FLUSH_IO);
+
 		/*
 		 * Block if we have unsent data.  XXX For logical replication, let
 		 * WalSndWaitForWal() handle any other blocking; idle receivers need
diff --git a/src/test/recovery/t/001_stream_rep.pl b/src/test/recovery/t/001_stream_rep.pl
index ee57d234c86..aea32f68b79 100644
--- a/src/test/recovery/t/001_stream_rep.pl
+++ b/src/test/recovery/t/001_stream_rep.pl
@@ -42,6 +42,9 @@ $node_standby_2->init_from_backup($node_standby_1, $backup_name,
 	has_streaming => 1);
 $node_standby_2->start;
 
+# To check that an active walsender updates its IO statistics below.
+$node_primary->safe_psql('postgres', "SELECT pg_stat_reset_shared('io')");
+
 # Create some content on primary and check its presence in standby nodes
 $node_primary->safe_psql('postgres',
 	"CREATE TABLE tab_int AS SELECT generate_series(1,1002) AS a");
@@ -69,6 +72,17 @@ ALTER EVENT TRIGGER on_login_trigger ENABLE ALWAYS;
 $node_primary->wait_for_replay_catchup($node_standby_1);
 $node_standby_1->wait_for_replay_catchup($node_standby_2, $node_primary);
 
+# Ensure an active walsender updates its IO statistics.
+is( $node_primary->safe_psql(
+		'postgres',
+		qq(SELECT sum(reads) > 0
+       FROM pg_catalog.pg_stat_io
+       WHERE backend_type = 'walsender'
+       AND object = 'wal')
+	),
+	qq(t),
+	"Check that the walsender updates its IO statistics");
+
 my $result =
   $node_standby_1->safe_psql('postgres', "SELECT count(*) FROM tab_int");
 print "standby 1: $result\n";
-- 
2.34.1

>From 42cce18fd4565118c430bdd422f509361c8f86e5 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <[email-masked]>
Date: Mon, 3 Mar 2025 10:02:40 +0000
Subject: [PATCH v3 2/2] Add a new backend_has_iostats global variable

It behaves as the existing have_iostats and replace the existing pg_memory_is_all_zeros()
calls in flushing backend stats functions. Indeed some perf measurements report
that those calls are responsible for a large part of the flushing backend stats
functions.
---
 src/backend/utils/activity/pgstat_backend.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)
 100.0% src/backend/utils/activity/

diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c
index a9343b7b59e..0ae3a1d4d23 100644
--- a/src/backend/utils/activity/pgstat_backend.c
+++ b/src/backend/utils/activity/pgstat_backend.c
@@ -37,6 +37,7 @@
  * memory allocation.
  */
 static PgStat_BackendPending PendingBackendStats;
+static bool backend_has_iostats = false;
 
 /*
  * Utility routines to report I/O stats for backends, kept here to avoid
@@ -68,6 +69,8 @@ pgstat_count_backend_io_op(IOObject io_object, IOContext io_context,
 
 	PendingBackendStats.pending_io.counts[io_object][io_context][io_op] += cnt;
 	PendingBackendStats.pending_io.bytes[io_object][io_context][io_op] += bytes;
+
+	backend_has_iostats = true;
 }
 
 /*
@@ -150,8 +153,7 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
 	 * statistics.  In this case, avoid unnecessarily modifying the stats
 	 * entry.
 	 */
-	if (pg_memory_is_all_zeros(&PendingBackendStats.pending_io,
-							   sizeof(struct PgStat_PendingIO)))
+	if (!backend_has_iostats)
 		return;
 
 	shbackendent = (PgStatShared_Backend *) entry_ref->shared_stats;
@@ -182,6 +184,8 @@ pgstat_flush_backend_entry_io(PgStat_EntryRef *entry_ref)
 	 * Clear out the statistics buffer, so it can be re-used.
 	 */
 	MemSet(&PendingBackendStats.pending_io, 0, sizeof(PgStat_PendingIO));
+
+	backend_has_iostats = false;
 }
 
 /*
@@ -198,8 +202,7 @@ pgstat_flush_backend(bool nowait, bits32 flags)
 	if (!pgstat_tracks_backend_bktype(MyBackendType))
 		return false;
 
-	if (pg_memory_is_all_zeros(&PendingBackendStats,
-							   sizeof(struct PgStat_BackendPending)))
+	if (!pgstat_backend_have_pending_cb())
 		return false;
 
 	entry_ref = pgstat_get_entry_ref_locked(PGSTAT_KIND_BACKEND, InvalidOid,
@@ -222,8 +225,7 @@ pgstat_flush_backend(bool nowait, bits32 flags)
 bool
 pgstat_backend_have_pending_cb(void)
 {
-	return (!pg_memory_is_all_zeros(&PendingBackendStats,
-									sizeof(struct PgStat_BackendPending)));
+	return backend_has_iostats;
 }
 
 /*
-- 
2.34.1

---

From: Michael Paquier <[email-masked]>
Date: Mon, 10 Mar 2025 09:23:50 +0900

> that could still make sense to reduce the frequency of the flush calls, thoughts?

Okay.  The number of reports really worry me anyway as proposed in the
patch.  It's not really complicated to see dozens of pgstats calls in
a single millisecond in the WAL sender, even with some of the
regression tests.  That's more present in the logirep scenarios, it
seems..

For example, here are some numbers based on some elog() calls planted
in walreceiver.c, looking at the logs of the TAP tests.  Through a
single check-world, I am able to gather the following information:
- The location proposed by the patch leads to 680k stats reports,
in WalSndLoop() because the "Block if we have unsent data".  Some
tests show quite some contention.
- A different location, in WalSndLoop() just before WalSndWait() leads
to 59k calls.  And there's some contention in the logirep tests..
- I've spotted a third candidate which looks pretty solid, actually:
WalSndWaitForWal() before WalSndWait().  This leads to 2.9k reports in
the whole test suite, with much less contention in the reports.  These
can still be rather frequent, up to ~50 calls per seconds, but that's
really less than the two others.

Stats data is useful as long as it is possible to get an idea of how
the system behaves, particularly with a steady workload.  More
frequent reports are useful for spikey data detection, showing more
noise.  Still, too many reports may cause the part gathering the
reports to become a bottleneck, while we want it to offer hints about
bottlenecks.  So I would argue in favor of a more conservative choice
in the back branches than what the patch is proposing.  Choice 3 i'm
quoting above is tempting by design: not too much, still frequent
enough to offer enough relevant information in the stats.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 10 Mar 2025 14:52:42 +0000

Hi,

On Mon, Mar 10, 2025 at 09:23:50AM +0900, Michael Paquier wrote:
> in the back branches than what the patch is proposing.

Yeah, fully agree. Anyway, having so many frequent stats reports makes little sense.

> Choice 3 i'm

Yeah, I also agree that we should reduce the number of "reports". I'll look
at the third option.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 11 Mar 2025 06:10:32 +0000

Hi,

> Yeah, I also agree that we should reduce the number of "reports". I'll look

WalSndWaitForWal() is being used only for logical walsender. So we'd need to 
find another location for the physical walsender case. One option is to keep the
WalSndLoop() location and control the reports frequency.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 13 Mar 2025 09:31:29 +0000

Hi,

> WalSndLoop() location and control the reports frequency.

I ended up with an idea in the same vein as your third one, but let's do it
in WalSndLoop() instead for both logical and physical walsenders. The idea
is to report the stats when the walsender is caught up or has pending data to
send. Also, we do flush only periodically (based on PGSTAT_MIN_INTERVAL).

Remarks/Questions:

1. maybe relying on PGSTAT_IDLE_INTERVAL would make more sense? In both case
PGSTAT_MIN_INTERVAL or PGSTAT_MIN_INTERVAL, I'm not sure there is a need to 
update the related doc.

2. bonus point to make it here is that we don't need an extra call to
GetCurrentTimestamp(). We simply assign the one that is already done to "now".

3. add to use poll_query_until() has I've seen failures on the CI whith this
new approach. Also moving the test far away from the stats reset to to minimize
the risk of polling for too long.

With the attached in place, the number of times the stats are being flushed
reduce drastically but still seems enough to get an accurate idea of the walsender
IO activity. Thoughts?

Regards,

---

From: Xuneng Zhou <[email-masked]>
Date: Thu, 13 Mar 2025 19:33:24 +0800

Forgot to cc...

---------- Forwarded message ---------
发件人： Xuneng Zhou <[email-masked]>
Date: 2025年3月13日周四 19:15
Subject: Re: [BUG]: the walsender does not update its IO statistics until
it exits
To: Bertrand Drouvot <[email-masked]>

Hi,

Thanks for working on this! I'm glad to see that the patch (
https://www.postgresql.org/message-id/flat/Z3zqc4o09dM/[email-masked])
has been committed.

Regarding patch 0001, the optimization in pgstat_backend_have_pending_cb
looks good:

bool

Additionally, the function pgstat_flush_backend includes the check:

However, I think we might need to revise the comment (and possibly the
function name) for clarity:

/*
 * Check if there are any backend stats waiting to be flushed.
 */

Originally, this function was intended to check multiple types of backend
statistics, which made sense when PendingBackendStats was the centralized
structure for various pending backend stats. However, since
PgStat_PendingWalStats was removed from PendingBackendStats earlier, and
now this patch introduces the backend_has_iostats variable, the scope of
this function appears even narrower. This narrowed functionality no longer
aligns closely with the original function name and its associated comment.

---

From: Xuneng Zhou <[email-masked]>
Date: Thu, 13 Mar 2025 19:35:24 +0800

Sorry, forgot to cc this one too.

---------- Forwarded message ---------
Date: 2025年3月13日周四 19:29

Hi,
Given that PGSTAT_MIN_INTERVAL is the go-to setting for stats flushes
 * Unless called with 'force', pending stats updates are flushed happen once
 * per PGSTAT_MIN_INTERVAL (1000ms). When not forced, stats flushes do not
 * block on lock acquisition, except if stats updates have been pending for
 * longer than PGSTAT_MAX_INTERVAL (60000ms).
 *
 * Whenever pending stats updates remain at the end of pgstat_report_stat()
a
 * suggested idle timeout is returned. Currently this is always
 * PGSTAT_IDLE_INTERVAL (10000ms). Callers can use the returned time to set
up
 * a timeout after which to call pgstat_report_stat(true), but are not
 * required to do so.

> PGSTAT_MIN_INTERVAL or PGSTAT_MIN_INTERVAL, I'm not sure there is a need to
PGSTAT_IDLE_INTERVAL seems to reduce the frequency even more.

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 13 Mar 2025 13:18:45 +0000

Hi,

> Regarding patch 0001, the optimization in pgstat_backend_have_pending_cb

Thanks for looking at it!

>  * Check if there are any backend stats waiting to be flushed.

The comment is not exactly this one on the current HEAD, it looks like that you're
looking at a previous version of the core code.

> aligns closely with the original function name and its associated comment.

I don't think so, as since 76def4cdd7c, a pgstat_backend_wal_have_pending() check
has been added to pgstat_backend_have_pending_cb(). You're probably looking at
a version prior to 76def4cdd7c.

This particular sub-patch needs a rebase though, done in the attached. 0001
remains unchanged as compared to the v4 one just shared up-thread. If 0001 goes
in, merging 0002 would be less beneficial (as compared to v3).

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Thu, 13 Mar 2025 13:19:51 +0000

Hi,

> PGSTAT_IDLE_INTERVAL seems to reduce the frequency even more.

Yeah, I think that PGSTAT_MIN_INTERVAL is the one to use (that's why that's the 
one the patch is using). I just mentioned PGSTAT_IDLE_INTERVAL as an open door
for conversation in case one prefers a "larger" frequency. 

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Tue, 18 Mar 2025 17:11:02 +0900

> in, merging 0002 would be less beneficial (as compared to v3).

PgStat_PendingIO is already quite large, could get larger, and the
backend stats would be a natural victim of that.  What you are
proposing in v4-0002 is beneficial in any case outside of the specific
problem of this thread with WAL senders.  It is new code on HEAD and
pgstat_flush_backend_entry_io() being the only entry point where IO
data is appended makes it easy to reason about.  Just to say that I am
planning to apply this part on HEAD.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 18 Mar 2025 09:51:14 +0000

Hi,

On Tue, Mar 18, 2025 at 05:11:02PM +0900, Michael Paquier wrote:
> problem of this thread with WAL senders.

Yeah, I also think it makes sense independently of what has been discussed
in this thread.

> Just to say that I am planning to apply this part on HEAD.

Thanks!

Regards,

---

From: Xuneng Zhou <[email-masked]>
Date: Tue, 18 Mar 2025 19:14:12 +0800

Hi,

I performed some tests using elog(no so sure whether this is the proper way
to do it) to monitor the new method. Here are the findings:

• With PGSTAT_MIN_INTERVAL set to 1000ms, the number of flush reports was
reduced to approximately 40–50 during the installcheck test suite.

• With PGSTAT_IDLE_INTERVAL set to 10000ms, the reports dropped to fewer
than 5.

• In contrast, the previous approach—flushing after every
WalSndKeepaliveIfNecessary()—resulted in roughly 50,000 flushes.

This reduction is significant, so the overhead from the flush reports is no
longer a concern. However, we still need to determine whether this
frequency is sufficient to capture the system’s state during periods of
high WAL activity. Based on my tests, using PGSTAT_MIN_INTERVAL seems to
provide a better balance than PGSTAT_IDLE_INTERVAL.

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 18 Mar 2025 14:45:15 +0000

Hi,

On Tue, Mar 18, 2025 at 07:14:12PM +0800, Xuneng Zhou wrote:
> Hi,
> 
> I performed some tests using elog

Thanks for the testing!

> (no so sure whether this is the proper way
> to do it) to monitor the new method.

Well that simple enough and that works well if the goal is just to "count" ;-)

> Here are the findings:
> longer a concern.

Yeah I did observe and do think the same.

> However, we still need to determine whether this
> high WAL activity.

I think that reporting at PGSTAT_MIN_INTERVAL is fine and more than enough. I
mean, I 'm not sure that there is a real use case to query the statistics related
view at more than a second interval anyway. Or are you concerned that we may not
enter the "When the WAL sender is caught up or has pending data to send" frequently
enough?

> Based on my tests, using PGSTAT_MIN_INTERVAL seems to

Same here.

Regards,

---

From: Michael Paquier <[email-masked]>
Date: Wed, 19 Mar 2025 08:19:42 +0900

> Thanks!

Done that part for now.  It will be possible to look at the bug fix
after the release freeze as it impacts stable branches as well.
--
Michael

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 19 Mar 2025 09:53:16 +0800

Hi,

I am OK with PGSTAT_MIN_INTERVAL. It has been used for flushing other
statistics as well. And monitoring systems are generally configured to poll
at one-second or longer intervals.

> view at more than a second interval anyway.

I think these two conditions are good too. In a busy system, they are met
frequently, so the flush routine will be executed at least once every
second. Conversely, when WAL generation is low, there's simply less data to
record, and the flush frequency naturally decreases.

> Or are you concerned that we may not

---

From: Michael Paquier <[email-masked]>
Date: Wed, 19 Mar 2025 14:26:47 +0900

> record, and the flush frequency naturally decreases.

Hmm, yeah, perhaps this is acceptable.  The changes in pgstat.c seem
inconsistent, though, only moving the min interval while the max and
idle times stay around.

This also make me wonder if we should work towards extending
pgstat_report_stat(), so as we save in GetCurrentTimestamp() while
making the internals still local to pgstat.c.  Or perhaps not in the
scope of a backpatchable design.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 19 Mar 2025 05:26:54 +0000

Hi,

On Wed, Mar 19, 2025 at 08:19:42AM +0900, Michael Paquier wrote:
> Done that part for now.

Thanks!

> It will be possible to look at the bug fix

Yes, let's do that. Adding a reminder on my side.

Regards,

---

From: Bertrand Drouvot <[email-masked]>
Date: Wed, 19 Mar 2025 05:50:28 +0000

Hi,

> inconsistent, though, only moving the min interval while the max and

That's right. OTOH that sounds weird to move the others 2: that would create 
wider visibility for them without real needs. That's not a big issue,
but could impact extensions or friends that would start using those should we
change their values in the future. 

Another option could be to create a dedicated one in walsender.c but I'm not
sure I like it more.

> scope of a backpatchable design.

I see. I think I'd vote for making the same changes on HEAD and the STABLE branches
as a first step. And then think about a new "design" that could be part of the
ideas suggested by Andres in [1]. Thoughts?

Regards,

[1]: https://www.postgresql.org/message-id/erpzwxoptqhuptdrtehqydzjapvroumkhh7lc6poclbhe7jk7l%40l3yfsq5q4pw7

---

From: Xuneng Zhou <[email-masked]>
Date: Wed, 19 Mar 2025 16:00:49 +0800

Hi,
Moving the other two provides a more complete view of the settings. For
newcomers(like me) to the codebase, seeing all three related values in one
place helps avoid a narrow view of the settings.

But I am not sure that I understand the cons of this well.

> change their values in the future.

 The current name of the min interval appears consistent within the context
of the surrounding code in this patch.

---

From: Michael Paquier <[email-masked]>
Date: Mon, 24 Mar 2025 08:41:20 +0900

> But I am not sure that I understand the cons of this well.

While I don't disagree with the use of a hardcoded interval of time to
control timing the flush of the WAL sender stats, do we really need to
rely on the timing defined by pgstat.c?  Wouldn't it be simpler to
assign one in walsender.c and pick up a different, perhaps higher,
value?

At the end the timestamp calculations are free because we can rely on
the existing call of GetCurrentTimestamp() for the physical WAL
senders to get an idea of the current time.  For the logical WAL
senders, perhaps we'd better do the reports in WalSndWaitForWal(),
actually.  There is also a call to GetCurrentTimestamp() that we can
rely on in this path.
--
Michael

---

From: Bertrand Drouvot <[email-masked]>
Date: Mon, 31 Mar 2025 13:05:24 +0000

Hi,

On Mon, Mar 24, 2025 at 08:41:20AM +0900, Michael Paquier wrote:
> rely on the timing defined by pgstat.c?

No but I thought it could make sense.

> Wouldn't it be simpler to

I don't have a strong opinion on it so done as suggested above in the attached.

I think that the 1s value is fine because: 1. it is consistent with 
PGSTAT_MIN_INTERVAL and 2. it already needs that the sender is caught up or
has pending data to send (means it could be higher than 1s already). That said,
I don't think that would hurt if you think of a higher value.

> senders to get an idea of the current time.

Yup

> For the logical WAL

I think it's better to flush the stats in a shared code path. I think it's
easier to maintain and that there is no differences between logical and
physical walsenders that would justify to flush the stats in specific code
paths.

Regards,