Re: shared-memory based stats collector - v70

Hi,

On 2022-04-06 13:31:31 +0200, Alvaro Herrera wrote:
> Just skimming a bit here ...

Thanks!

> On 2022-Apr-05, Andres Freund wrote:
>
> > From 0532b869033595202d5797b148f22c61e4eb4969 Mon Sep 17 00:00:00 2001
> > From: Andres Freund <andres(at)anarazel(dot)de>
> > Date: Mon, 4 Apr 2022 16:53:16 -0700
> > Subject: [PATCH v70 10/27] pgstat: store statistics in shared memory.
>
> > + <entry><literal>PgStatsData</literal></entry>
> > + <entry>Waiting fo shared memory stats data access</entry>
> > + </row>
>
> Typo "fo" -> "for"

Oh, oops. I had fixed that in the wrong patch.

> > @@ -5302,7 +5317,9 @@ StartupXLOG(void)
> > performedWalRecovery = true;
> > }
> > else
> > + {
> > performedWalRecovery = false;
> > + }
>
> Why? :-)

Damage from merging two commits yesterday. I'd left open where exactly we'd
reset stats, with the "main commit" implementing the current behaviour more
closely, and then a followup commit implementing something a bit
better. Nobody seemed to argue for keeping the behaviour 1:1, so I merged
them. Without removing the parens again :)

> Why give pgstat_get_entry_ref the responsibility of initializing
> created_entry to false? The vast majority of callers don't care about
> that flag; it seems easier/cleaner to set it to false in
> pgstat_init_function_usage (the only caller that cares that I could
> find) before calling pgstat_prep_pending_entry.

It's annoying to have to initialize it, I agree. But I think it's bugprone for
the caller to know that it has to be pre-initialized to false.

> (I suggest pgstat_prep_pending_entry should have a comment line stating
> "*created_entry, if not NULL, is set true if the entry required to be
> created.", same as pgstat_get_entry_ref.)

Added something along those lines.

Greetings,

Andres Freund