Hi, On Thu, Dec 05, 2024 at 05:13:13PM +0900, Michael Paquier wrote: > On Thu, Dec 05, 2024 at 07:37:27AM +0000, Bertrand Drouvot wrote: > > That said, I think that's worth to update the comment a bit (like in the > > attached?) as I think that answers a legitimate question someone could have > > while > > reading this code. > > > > - /* we may have some "dropped" entries not yet removed, skip > > them */ > > + /* > > + * We may have some "dropped" entries not yet removed, skip > > them as > > + * it's not worth taking down the server for this. > > + */ > > Perhaps this should provide some details, like the fact that we don't > expect the server to still have references to entries that are dropped > at shutdown when writing the stats file as all the backends and/or > auxiliary processes should have done this cleanup before they are > gone.
Okay, attached a more elaborated comment. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From 88dcd8f52e2146dca254589e31a92a5dbae8f8a7 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <bertranddrouvot...@gmail.com> Date: Thu, 5 Dec 2024 07:19:34 +0000 Subject: [PATCH v2] Improve comment around code that should not be reached in pgstat_write_statsfile() Make it clear that this code should not be reached and that the branch is there only because that would not be worth taking down the server. --- src/backend/utils/activity/pgstat.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 100.0% src/backend/utils/activity/ diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index 6f8d237826..7533dea640 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1655,7 +1655,14 @@ pgstat_write_statsfile(XLogRecPtr redo) CHECK_FOR_INTERRUPTS(); - /* we may have some "dropped" entries not yet removed, skip them */ + /* + * We should not see any "dropped" entries when writing the stats + * file, as all backends and auxiliary processes should have cleaned + * up their references before they terminated. However, since we're + * already shutting down, it's not worth crashing the server over any + * potential cleanup issues, so we simply skip such entries if + * encountered. + */ Assert(!ps->dropped); if (ps->dropped) continue; -- 2.34.1
