Re: shared-memory based stats collector

At Fri, 12 Mar 2021 23:33:05 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>
>
> On 2021/03/12 17:24, Kyotaro Horiguchi wrote:
> > 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.
>
> Thanks! If we come up with better name, let's rename the function
> later.

Ok.

> > # 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.
>
> That's an idea. But the similar calculation using that cast is used in
> other places (e.g., in pgarch_MainLoop()), so I'm thinking that it's
> better not to change that...

Mmm. I'm fine with it:( (:p)

> > And the last_pgarch_start_time is accessed only in the function. I
> > moved it to inside the function.
>
> OK.
>
>
> >
> >> + /*
> >> + * 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.
>
> OK.
>
> Thanks for updating the patch! I applied some minor changes into your
> patch.
> Attached is the updated version of the patch. I'm thinking to commit
> this version.

Thanks for committing this! I'm very happy to see this reduces the
size of this patchset.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center