Content-Type: multipart/mixed; boundary="------------5wkxFQWSQwLm2tUN6ROL0Iux"
Message-ID: <5911c537-5665-4e93-91fa-de5465df487d@postgrespro.ru>
Date: Sat, 7 Dec 2024 12:31:46 +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: 
 <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>
Content-Language: en-US
From: "Anton A. Melnikov" <a.melnikov@postgrespro.ru>
In-Reply-To: <Z1G1JiO2TXwr1Xa8@ip-10-97-1-34.eu-west-3.compute.internal>
Archived-At: 
 <https://www.postgresql.org/message-id/5911c537-5665-4e93-91fa-de5465df487d%40postgrespro.ru>
Precedence: bulk

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

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

RnJvbSAxYmVjYmZhMWM4NzMxNzRmNzMxMWFkNTc1NjlhNGNlOGE5ZTNhOWM4IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiAiQW50b24gQS4gTWVsbmlrb3YiIDxhLm1lbG5pa292
QHBvc3RncmVzcHJvLnJ1PgpEYXRlOiBTYXQsIDcgRGVjIDIwMjQgMTI6MDA6MTAgKzAzMDAK
U3ViamVjdDogW1BBVENIXSBBZGQgd2FybmluZyBhYm91dCBkcm9wcGVkIHN0YXQgZW50cmll
cyBhdCBzZXJ2ZXIgc2h1dGRvd24uCgotLS0KIHNyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5
L3Bnc3RhdC5jIHwgNyArKysrKysrCiAxIGZpbGUgY2hhbmdlZCwgNyBpbnNlcnRpb25zKCsp
CgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvdXRpbHMvYWN0aXZpdHkvcGdzdGF0LmMgYi9z
cmMvYmFja2VuZC91dGlscy9hY3Rpdml0eS9wZ3N0YXQuYwppbmRleCA3NTMzZGVhNjQwNy4u
YTE4NGMyOWY1ZWEgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L3Bn
c3RhdC5jCisrKyBiL3NyYy9iYWNrZW5kL3V0aWxzL2FjdGl2aXR5L3Bnc3RhdC5jCkBAIC0x
NjY1LDcgKzE2NjUsMTQgQEAgcGdzdGF0X3dyaXRlX3N0YXRzZmlsZShYTG9nUmVjUHRyIHJl
ZG8pCiAJCSAqLwogCQlBc3NlcnQoIXBzLT5kcm9wcGVkKTsKIAkJaWYgKHBzLT5kcm9wcGVk
KQorCQl7CisJCQlQZ1N0YXRfSGFzaEtleSBrZXkgPSBwcy0+a2V5OworCQkJZWxvZyhXQVJO
SU5HLCAiZm91bmQgbm9uLWRlbGV0ZWQgc3RhdHMgZW50cnkgJXUvJXUvJWxsdSIKKwkJCQkJ
CSAgImF0IHNlcnZlciBzaHV0ZG93biIsCisJCQkJCQkgICBrZXkua2luZCwga2V5LmRib2lk
LAorCQkJCQkJICAgKHVuc2lnbmVkIGxvbmcgbG9uZykga2V5Lm9iamlkKTsKIAkJCWNvbnRp
bnVlOworCQl9CiAKIAkJLyoKIAkJICogVGhpcyBkaXNjYXJkcyBkYXRhIHJlbGF0ZWQgdG8g
Y3VzdG9tIHN0YXRzIGtpbmRzIHRoYXQgYXJlIHVua25vd24KLS0gCjIuNDcuMAoK

--------------5wkxFQWSQwLm2tUN6ROL0Iux--