From: Anton A. Melnikov <a.melnikov@postgrespro.ru>
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>
Cc: Drouvot, Bertrand <bdrouvot@amazon.com>
Cc: Greg Stark <stark@mit.edu>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Melanie Plageman <melanieplageman@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Cc: David G. Johnston <david.g.johnston@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: shared-memory based stats collector - v70
Date: Sat, 7 Dec 2024 12:31:46 +0300
Message-ID: <5911c537-5665-4e93-91fa-de5465df487d@postgrespro.ru> (raw)
In-Reply-To: <Z1G1JiO2TXwr1Xa8@ip-10-97-1-34.eu-west-3.compute.internal>
References: <CAM-w4HNm06PcCM40-kY+jLgD_Tqjr3ymBy8qErfOvWqQyJE+6Q@mail.gmail.com>
<20220818195124.c7ipzf6c5v7vxymc@awork3.anarazel.de>
<5bfcf1a5-4224-9324-594b-725e704c95b1@amazon.com>
<a13e8cdf-b97a-4ecb-8f42-aaa367974e29@postgrespro.ru>
<wurgoxcnwm44fdysbgkyucjedwp4jxqjzpw2mk2meqjnhdmlao@fldawjm5fmp2>
<b1dbea9a-f433-4dd7-85cd-d706cfdc67fa@postgrespro.ru>
<Z1B0R7TLMVsZlONS@ip-10-97-1-34.eu-west-3.compute.internal>
<Z1E9j81TeIQj0mha@paquier.xyz>
<Z1FYN+t8dNX6MjZn@ip-10-97-1-34.eu-west-3.compute.internal>
<Z1FgmTp30aZ3o-c1@paquier.xyz>
<Z1G1JiO2TXwr1Xa8@ip-10-97-1-34.eu-west-3.compute.internal>
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
Attachments:
[text/x-patch] v2-0002-Add-warning-about-dropped-stat-entries.patch (966B, ../5911c537-5665-4e93-91fa-de5465df487d@postgrespro.ru/2-v2-0002-Add-warning-about-dropped-stat-entries.patch)
download | inline diff:
From 1becbfa1c873174f7311ad57569a4ce8a9e3a9c8 Mon Sep 17 00:00:00 2001
From: "Anton A. Melnikov" <a.melnikov@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.cindex 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
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: pgsql-hackers@postgresql.org
Cc: a.melnikov@postgrespro.ru, bertranddrouvot.pg@gmail.com, michael@paquier.xyz, andres@anarazel.de, bdrouvot@amazon.com, stark@mit.edu, tgl@sss.pgh.pa.us, melanieplageman@gmail.com, horikyota.ntt@gmail.com, pryzby@telsasoft.com, thomas.munro@gmail.com, david.g.johnston@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector - v70
In-Reply-To: <5911c537-5665-4e93-91fa-de5465df487d@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox