Lift shutdown assertion in pgstats for WAL senders

Before v17, WAL senders can shut down after the checkpointer.  If a WAL
sender still has pending statistics when the checkpointer has already
exited, its shutdown callback may attempt to report those statistics and
trigger assertions in pgstats.  In that case, the pending statistics are
lost.

This commit adjusts the assertion handling so that attempts to report
pending WAL sender statistics after the checkpointer has completed its
final stats flush are skipped.

Preserving the existing assertion would require backpatching an
equivalent of 87a6690cc69, ensuring that the checkpointer is always the
last process to exit.  Such a change would be considerably more invasive
and risky for stable branches because it alters the shutdown sequence,
and the consequence is only some loss of stats data for the WAL sender.

This assertion failure was periodically detected in the buildfarm,
leading to spurious failures.

Reported-by: Alexander Lakhin <exclusion@gmail.com>
Reviewed-by: Xuneng Zhou <xunengzhou@gmail.com>
Discussion: https://postgr.es/m/18158-88f667028dbc7e7b@postgresql.org
Backpatch-through: 15-17

diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index ab2f5cf635e2e55b8cd9768f01a89002909c454e..458b6f617a0b62a8bfeee3d972c9510c76b64233 100644 (file)
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -98,6 +98,7 @@
 #include "lib/dshash.h"
 #include "pgstat.h"
 #include "port/atomics.h"
+#include "replication/walsender.h"
 #include "storage/fd.h"
 #include "storage/ipc.h"
 #include "storage/lwlock.h"
@@ -519,8 +520,12 @@ pgstat_shutdown_hook(int code, Datum arg)
 
        pgstat_report_stat(true);
 
-       /* there shouldn't be any pending changes left */
-       Assert(dlist_is_empty(&pgStatPending));
+       /*
+        * There shouldn't be any pending changes left, unless this is a WAL
+        * sender that would shut down after the checkpointer has flushed the
+        * stats.
+        */
+       Assert(dlist_is_empty(&pgStatPending) || am_walsender);
        dlist_init(&pgStatPending);
 
        pgstat_detach_shmem();
@@ -611,7 +616,13 @@ pgstat_report_stat(bool force)
         * assert that before the checks above, as there is an unconditional
         * pgstat_report_stat() call in pgstat_shutdown_hook() - which at least
         * the process that ran pgstat_before_server_shutdown() will still call.
+        *
+        * WAL senders would be shut down after the checkpointer and may still
+        * have stats.  Skip them.
         */
+       if (pgStatLocal.shmem->is_shutdown && am_walsender)
+               return 0;
+
        Assert(!pgStatLocal.shmem->is_shutdown);
 
        if (force)