Re: shared-memory based stats collector - v70

Hi,

On Mon, Dec 09, 2024 at 02:39:58PM +0900, Michael Paquier wrote:
> On Sat, Dec 07, 2024 at 12:31:46PM +0300, Anton A. Melnikov wrote:
> > 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.
>
> Yep, so applied v2-0001 to document that, and backpatched it as it is
> kind of important to know about.
>
> > Maybe it's worth adding a warning as well,
> > similar to the one a few lines below in the code?
>
> Assert(!ps->dropped);
> if (ps->dropped)
> + {
> + PgStat_HashKey key = ps->key;
> + elog(WARNING, "found non-deleted stats entry %u/%u/%llu"
> + "at server shutdown",

There is a missing space. I think that should be " at server..." or "...%llu ".

> + key.kind, key.dboid,
> + (unsigned long long) key.objid);
> continue;
> + }
>
> /*
> * This discards data related to custom stats kinds that are unknown
>
> Not sure how to feel about this suggestion, though. This would
> produce a warning when building without assertions, but the assertion
> would likely let us more information with a trace during development,
> so..

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).

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com