Re: shared-memory based stats collector

At Fri, 12 Mar 2021 15:13:15 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
> On 2021/03/12 13:49, Kyotaro Horiguchi wrote:
> > I noticed that I accidentally removed the launch-suppression feature
> > that is to avoid frequent relaunching. That mechanism is needed on
> > the postmaster side. I added PgArchIsSuppressed() to do the same check
> > with the old pgarch_start() and make it called as a part of
> > PgArchStartupAllowed().
>
> You're right! At least for me the function name PgArchIsSuppressed()
> sounds not good to me. What about something like PgArchCanRestart()?

The reason for the name was some positive-meaning names came up with
me are confusing with PgArchStartupAllowed(). The name
PgArchCanRestart suggests that it's usable only when
restarting. However, the function requires to be called also when the
first launch since last_pgarch_start_time needs to be updated every
time archiver is launched.

Anyway in the attached the name is changed wihtout changing its usage.

# I don't like it uses "can" as "allowed" so much. The archiver
# actually can restart but is just inhibited to restart.

> This is not fault of this patch. But last_pgarch_start_time should be
> initialized with zero?

Right. I noticed that but forgot to fix it.

> + if ((curtime - last_pgarch_start_time) < PGARCH_RESTART_INTERVAL)
> + return true;
>
> Why did you remove the cast to unsigned int there?

The cast converts small negative values to large numbers, the code
looks like intending to allow archiver launched when curtime goes
behind than last_pgarch_start_time. That is the case the on-memory
data is corrupt. I'm not sure it's worth worrying and in the first
place if we want to care of the case we should explicitly compare the
operands of the subtraction. I did that in the attached.

And the last_pgarch_start_time is accessed only in the function. I
moved it to inside the function.

> + /*
> + * Advertise our latch that backends can use to wake us up while
> we're
> + * sleeping.
> + */
> + PgArch->pgprocno = MyProc->pgprocno;
>
> The comment should be updated?

Hmm. What is advertised is our pgprocno.. Fixed.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center