public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: Xuneng Zhou <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: [email protected]
Subject: Re: BUG #18158: Assert in pgstat_report_stat() fails when a backend shutting down with stats pending
Date: Fri, 5 Jun 2026 11:14:31 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CABPTF7VgzdBP_ZPKf2eZ79ry0fVKzcgCkB5gy60HGXCy0EQb0w@mail.gmail.com>
References: <[email protected]>
<[email protected]>
<CABPTF7XdDoLAoLKo7pOHmSqHniHf46Pw8Z=iqNx2uYi4J5ixBA@mail.gmail.com>
<[email protected]>
<CABPTF7VgzdBP_ZPKf2eZ79ry0fVKzcgCkB5gy60HGXCy0EQb0w@mail.gmail.com>
On Thu, May 14, 2026 at 03:19:34PM +0800, Xuneng Zhou wrote:
> I agree that applying the change wholesale could be too invasive. If
> we decide not to address the ordering issue, removing this assertion
> seems reasonable, since the non-assert path does not actually provide
> the required guarantee.
Attached is a patch for 15~17 that is able to achieve this goal:
- Lift the assertion on shutdown for WAL senders, but keep it
everywhere else.
- Secondary trick in pgstat_shutdown_hook to cope with the fact that
some stats make still be around at shutdown for a WAL sender.
I cannot reproduce the failure re-using the trick sent by Alexander at
the top of the thread.
Thoughts or objections?
--
Michael
From 62bae7f32ea095e0130af762bdf6ef39a7c5d388 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Fri, 5 Jun 2026 10:42:47 +0900
Subject: [PATCH] Lift shutdown assertion in pgstats for WAL senders
Backpatch-through: 15-17
---
src/backend/utils/activity/pgstat.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 33025cf4aad3..b7cf6fa88ff4 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -97,6 +97,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"
@@ -516,8 +517,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();
@@ -608,7 +613,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)
--
2.54.0
Attachments:
[text/plain] 0001-Lift-shutdown-assertion-in-pgstats-for-WAL-senders.patch (1.7K, 2-0001-Lift-shutdown-assertion-in-pgstats-for-WAL-senders.patch)
download | inline diff:
From 62bae7f32ea095e0130af762bdf6ef39a7c5d388 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
Date: Fri, 5 Jun 2026 10:42:47 +0900
Subject: [PATCH] Lift shutdown assertion in pgstats for WAL senders
Backpatch-through: 15-17
---
src/backend/utils/activity/pgstat.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 33025cf4aad3..b7cf6fa88ff4 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -97,6 +97,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"
@@ -516,8 +517,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();
@@ -608,7 +613,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)
--
2.54.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
view thread (12+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: BUG #18158: Assert in pgstat_report_stat() fails when a backend shutting down with stats pending
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox