Re: shared-memory based stats collector

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.

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

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

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION