agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: 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: Tue, 10 Dec 2024 10:44:29 +0300
Message-ID: <0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@postgrespro.ru> (raw)
In-Reply-To: <Z1fi3nAoI1pXpshr@ip-10-97-1-34.eu-west-3.compute.internal>
References: <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>
<5911c537-5665-4e93-91fa-de5465df487d@postgrespro.ru>
<Z1aCrhT9wl6p2g9t@paquier.xyz>
<Z1akatx3lLgm+nF+@ip-10-97-1-34.eu-west-3.compute.internal>
<Z1eRTELjXqieX0mI@paquier.xyz>
<Z1fi3nAoI1pXpshr@ip-10-97-1-34.eu-west-3.compute.internal>
Hi!
On 09.12.2024 11:03, Bertrand Drouvot wrote:
> There is a missing space. I think that should be " at server..." or "...%llu ".
Thanks for pointing this out. In the other code the elog messages are all on one line,
regardless of their length. Did the same in v3.
On 10.12.2024 09:42, Bertrand Drouvot wrote:
> On Tue, Dec 10, 2024 at 09:54:36AM +0900, Michael Paquier wrote:
>> On Mon, Dec 09, 2024 at 08:03:54AM +0000, Bertrand Drouvot wrote:
>>> Right. OTOH I think that could help the tap test added in da99fedf8c to not
>>> rely on assert enabled build (the tap test could "simply" check for the
>>> WARNING in the logfile instead).
>>
>> That's true. Still, the coverage that we have is also enough for
>> assert builds, which is what the test is going to run with most of the
>> time anyway.
>
> Yeah, that's fine by me and don't see the added value of the WARNING then.
Agreed that this WARNING has no additional value for testing purposes
at pgfarm or ci. Assert is better.
My logic was different.
It's clear that during normal server operation this code should be unreachable.
But we admit that in production deployments it can be executed in case of some bug
that is still unknown to us. Now it is done in such a way that in this case
the server simply skip it and won't notice about it. And no one will know that this happened.
But if there is a warning here, the information will remain in the server logs,
we can find out about it and we can try to reproduce similar behavior
in the testing environment and probably detect a hidden bug like in [1].
Thanks a lot for fixing this!
With the best regards,
--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
[1] https://www.postgresql.org/message-id/56bf8ff9-dd8c-47b2-872a-748ede82af99%40postgrespro.ru
Attachments:
[text/x-patch] v3-0002-Add-warning-about-dropped-stat-entries.patch (907B, ../0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@postgrespro.ru/2-v3-0002-Add-warning-about-dropped-stat-entries.patch)
download | inline diff:
From 3ff955ba7674c78a162a3c0243b28c5004768e07 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
---
src/backend/utils/activity/pgstat.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 7533dea6407..c1b5995f0ea 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1665,7 +1665,13 @@ 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.1
view thread (93+ messages)
Message-ID: <0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@postgrespro.ru>
Permalink: ../0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@postgrespro.ru/
Also on: postgresql.org/message-id/0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@postgrespro.ru
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: 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: <0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@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 agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox