Hi, On Thu, Dec 05, 2024 at 02:43:43PM +0900, Michael Paquier wrote: > On Wed, Dec 04, 2024 at 03:24:55PM +0000, Bertrand Drouvot wrote: > > Thanks! I've the feeling that something has to be fixed, see my comments in > > [1]. It might be that the failed assertion does not handle a "valid" > > scenario. > > > > [1]: > > https://www.postgresql.org/message-id/Z1BzI/eMTCOKA%2Bj6%40ip-10-97-1-34.eu-west-3.compute.internal > > It's really a case that should never be reached because it points to > an inconsistency in the interactions between the local entry cache in > a process and the central dshash it attempts to point to, so I don't > think that there is anything to change here.
Agree. > As Andres has mentioned, > it has a lot of value by acting as a safety guard in assert builds > without being annoying for production deployments. Fully agree. 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. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
>From cf422aceabbd17043d14e530540b0950256c2de2 Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot <bertranddrouvot...@gmail.com> Date: Thu, 5 Dec 2024 07:19:34 +0000 Subject: [PATCH v1] 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 | 5 ++++- 1 file changed, 4 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..39ade666a7 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -1655,7 +1655,10 @@ pgstat_write_statsfile(XLogRecPtr redo) CHECK_FOR_INTERRUPTS(); - /* 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. + */ Assert(!ps->dropped); if (ps->dropped) continue; -- 2.34.1
