Re: shared-memory based stats collector

On 2021/03/12 9:23, Kyotaro Horiguchi wrote:
> At Thu, 11 Mar 2021 15:33:52 +0900, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote in
>>
>>
>> On 2021/03/11 13:42, Kyotaro Horiguchi wrote:
>>> At Wed, 10 Mar 2021 19:21:00 -0800, Andres Freund <andres(at)anarazel(dot)de>
>>> wrote in
>>>> Hi,
>>>>
>>>> Two minor nits:
>>> Thanks for the comments!
>>>
>>>> On 2021-03-10 21:47:51 +0900, Fujii Masao wrote:
>>>>> +/* Shared memory area for archiver process */
>>>>> +typedef struct PgArchData
>>>>> +{
>>>>> + Latch *latch; /* latch to wake the archiver up */
>>>>> + slock_t mutex; /* locks this struct */
>>>>> +} PgArchData;
>>>>> +
>>>>
>>>> It doesn't really matter, but it'd be pretty trivial to avoid needing
>>>> a
>>>> spinlock for this kind of thing. Just store the pgprocno of the
>>>> archiver
>>>> in PgArchData.
>>> Looks promising.
>>
>> You mean that spinlock is not necessary by doing the followings?
>>
>> - Save pgprocno of archiver in PgArchData, instead of latch and mutex.
>> - Set PgArch->pgprocno at the startup of archiver
>> - Reset PgArch->pgprocno to INVALID_PGPROCNO at pgarch_die()
>> - XLogArchiveNotify() sets the latch (i.e.,
>> - &ProcGlobal->allProcs[PgArch->pgprocno].procLatch) if PgArch->pgprocno
>> - is not INVALID_PGPROCNO
>>
>> Right?
>
> I think it is right in a rough sketch.
>
>>>
>>>> While getting rid of the spinlock doesn't seem like a huge win, it
>>>> does
>>>> seem nicer that we'd automatically have a way to find data about the
>>>> archiver (e.g. pid).
>>> PGPROC GetAuxProcessInfo(AuxProcType type)?
>>
>> I don't think this new function is necessary.
>>
>> ISTM that Andres said that it's worth adding pgprocno into PgArch
>> because
>> which enables us to get the information about archiver more easily by
>> using that pgprocno. For example, we can get the pid of archiver by
>> &ProcGlobal->allProcs[PgArch->pgprocno].pid. That is, he thinks that
>> adding pgprocno has several merits. I agree that. Maybe I'm
>> misunderstanding
>> his comment, though...
>
> I meant some operation that converts an AuxProcType to a PGPROC * for
> any type of auxiliary process, but perhaps that's wrong. It should
> saying about the same thing to the previous paragraph and it seems
> that you're right.
>
>>>>> * checkpointer to exit as well, otherwise not. The archiver,
>>>>> * stats,
>>>>> * and syslogger processes are disregarded since they are not
>>>>> * connected to shared memory; we also disregard dead_end children
>>>>> * here. Walsenders are also disregarded, they will be terminated
>>>>> * later after writing the checkpoint record, like the archiver
>>>>> * process.
>>>>> */
>>>>
>>>> This comment in PostmasterStateMachine() is outdated now.
>>> Right. Will fix a bit later.
>
> I moved archiver from the current location to next to "walsenders"
> that is to be terminated along with archiver.
>
> The attached the only 0003 of the new version based on the last one
> from Fujii-san.

Thanks for updating the patch! But you forgot to add the changes
related to pgprocno into the patch?

Regards,

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