Re: shared-memory based stats collector - v67

Hi,

On 2022-03-23 17:27:50 +0900, Kyotaro Horiguchi wrote:
> At Mon, 21 Mar 2022 14:30:17 -0700, Andres Freund <andres(at)anarazel(dot)de> wrote in
> > > Right now we reset stats for replicas, even if we start from a shutdown
> > > checkpoint. That seems pretty unnecessary with this patch:
> > > - 0021-pgstat-wip-only-reset-pgstat-data-after-crash-re.patch
> >
> > Might raise this in another thread for higher visibility.
>
> + /*
> + * When starting with crash recovery, reset pgstat data - it might not be
> + * valid. Otherwise restore pgstat data. It's safe to do this here,
> + * because postmaster will not yet have started any other processes
> + *
> + * TODO: With a bit of extra work we could just start with a pgstat file
> + * associated with the checkpoint redo location we're starting from.
> + */
> + if (ControlFile->state == DB_SHUTDOWNED ||
> + ControlFile->state == DB_SHUTDOWNED_IN_RECOVERY)
> + pgstat_restore_stats();
> + else
> + pgstat_discard_stats();
> +
>
> Before there, InitWalRecovery changes the state to
> DB_IN_ARCHIVE_RECOVERY if it was either DB_SHUTDOWNED or
> DB_IN_PRODUCTION. So the stat seems like always discarded on standby.

Hm. I though it worked at some point. I guess there's a reason this commit is
a separate commit marked WIP ;)

> In the first place, I'm not sure it is valid that a standby from a
> cold backup takes over the stats from the primary.

I don't really see a reason not to use the stats in that case - we have a
correct stats file after all. But it doesn't seem too important. What I
actually find worth addressing is the case of standbys starting in
DB_SHUTDOWNED_IN_RECOVERY. Right now we always throw stats away after a
*graceful* restart of a standby, which doesn't seem great.

Greetings,

Andres Freund