Hi!
On 04.12.2024 18:24, 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
On 05.12.2024 08:43, Michael Paquier wrote:
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. 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.
Thanks a lot for for the detailed clarification!
Everything here became clear for me.
On 05.12.2024 11:13, 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.
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.
Completely agree that the original comment needs to be revised,
since it implies that it is normal for deleted entries to be here,
but it is not the case.
On 05.12.2024 17:13, Bertrand Drouvot wrote:
Okay, attached a more elaborated comment.
Looks good for me. Detailed and clear.
Will help to avoid unnecessary questions when reading this code.
Maybe it's worth adding a warning as well,
similar to the one a few lines below in the code?
Like in the patch attached?
With the best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
From 1becbfa1c873174f7311ad57569a4ce8a9e3a9c8 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <a.melni...@postgrespro.ru>
Date: Sat, 7 Dec 2024 12:00:10 +0300
Subject: [PATCH] Add warning about dropped stat entries at server shutdown.
---
src/backend/utils/activity/pgstat.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 7533dea6407..a184c29f5ea 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1665,7 +1665,14 @@ pgstat_write_statsfile(XLogRecPtr redo)
*/
Assert(!ps->dropped);
if (ps->dropped)
+ {
+ PgStat_HashKey key = ps->key;
+ elog(WARNING, "found non-deleted stats entry %u/%u/%llu"
+ "at server shutdown",
+ key.kind, key.dboid,
+ (unsigned long long) key.objid);
continue;
+ }
/*
* This discards data related to custom stats kinds that are unknown
--
2.47.0
