Content-Type: multipart/mixed; boundary="------------KI9cK9IPjHPGLordmGKZ6c0c"
Message-ID: <0ba6d37a-af09-4024-bbca-2dde0b0e3fa1@postgrespro.ru>
Date: Tue, 10 Dec 2024 10:44:29 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: shared-memory based stats collector - v70
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>,
 Michael Paquier <michael@paquier.xyz>
Cc: Andres Freund <andres@anarazel.de>,
 "Drouvot, Bertrand" <bdrouvot@amazon.com>, Greg Stark <stark@mit.edu>,
 Tom Lane <tgl@sss.pgh.pa.us>, Melanie Plageman <melanieplageman@gmail.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 Justin Pryzby <pryzby@telsasoft.com>, Thomas Munro <thomas.munro@gmail.com>,
 "David G. Johnston" <david.g.johnston@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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>
Content-Language: en-US
From: "Anton A. Melnikov" <a.melnikov@postgrespro.ru>
In-Reply-To: <Z1fi3nAoI1pXpshr@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: 
 <https://www.postgresql.org/message-id/0ba6d37a-af09-4024-bbca-2dde0b0e3fa1%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------KI9cK9IPjHPGLordmGKZ6c0c
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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
--------------KI9cK9IPjHPGLordmGKZ6c0c
Content-Type: text/x-patch; charset=UTF-8;
 name="v3-0002-Add-warning-about-dropped-stat-entries.patch"
Content-Disposition: attachment;
 filename="v3-0002-Add-warning-about-dropped-stat-entries.patch"
Content-Transfer-Encoding: base64

RnJvbSAzZmY5NTViYTc2NzRjNzhhMTYyYTNjMDI0M2IyOGM1MDA0NzY4ZTA3IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiAiQW50b24gQS4gTWVsbmlrb3YiIDxhLm1lbG5pa292
QHBvc3RncmVzcHJvLnJ1PgpEYXRlOiBTYXQsIDcgRGVjIDIwMjQgMTI6MDA6MTAgKzAzMDAK
U3ViamVjdDogW1BBVENIXSBBZGQgd2FybmluZwoKLS0tCiBzcmMvYmFja2VuZC91dGlscy9h
Y3Rpdml0eS9wZ3N0YXQuYyB8IDYgKysrKysrCiAxIGZpbGUgY2hhbmdlZCwgNiBpbnNlcnRp
b25zKCspCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0
LmMgYi9zcmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXQuYwppbmRleCA3NTMzZGVh
NjQwNy4uYzFiNTk5NWYwZWEgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2
aXR5L3Bnc3RhdC5jCisrKyBiL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L3Bnc3RhdC5j
CkBAIC0xNjY1LDcgKzE2NjUsMTMgQEAgcGdzdGF0X3dyaXRlX3N0YXRzZmlsZShYTG9nUmVj
UHRyIHJlZG8pCiAJCSAqLwogCQlBc3NlcnQoIXBzLT5kcm9wcGVkKTsKIAkJaWYgKHBzLT5k
cm9wcGVkKQorCQl7CisJCQlQZ1N0YXRfSGFzaEtleSBrZXkgPSBwcy0+a2V5OworCQkJZWxv
ZyhXQVJOSU5HLCAiZm91bmQgbm9uLWRlbGV0ZWQgc3RhdHMgZW50cnkgJXUvJXUvJWxsdSBh
dCBzZXJ2ZXIgc2h1dGRvd24iLAorCQkJCQkJICAga2V5LmtpbmQsIGtleS5kYm9pZCwKKwkJ
CQkJCSAgICh1bnNpZ25lZCBsb25nIGxvbmcpIGtleS5vYmppZCk7CiAJCQljb250aW51ZTsK
KwkJfQogCiAJCS8qCiAJCSAqIFRoaXMgZGlzY2FyZHMgZGF0YSByZWxhdGVkIHRvIGN1c3Rv
bSBzdGF0cyBraW5kcyB0aGF0IGFyZSB1bmtub3duCi0tIAoyLjQ3LjEKCg==

--------------KI9cK9IPjHPGLordmGKZ6c0c--