From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: andres@anarazel.de
Cc: gkokolatos@protonmail.com
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector
Date: Fri, 12 Mar 2021 10:03:31 +0900
Message-ID: <fa25c85a-2889-1ffc-84ff-2db2b2f7720e@oss.nttdata.com> (raw)
In-Reply-To: <20210312.092312.2023783779833776888.horikyota.ntt@gmail.com>
References: <20210311032100.sy25nnjowg7jjf5a@alap3.anarazel.de>
<20210311.134227.1280084378060460266.horikyota.ntt@gmail.com>
<9375d512-2c99-d951-a424-89c813b4df06@oss.nttdata.com>
<20210312.092312.2023783779833776888.horikyota.ntt@gmail.com>
On 2021/03/12 9:23, Kyotaro Horiguchi wrote:
> At Thu, 11 Mar 2021 15:33:52 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>
>>
>> On 2021/03/11 13:42, Kyotaro Horiguchi wrote:
>>> At Wed, 10 Mar 2021 19:21:00 -0800, Andres Freund <andres@anarazel.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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, horikyota.ntt@gmail.com, andres@anarazel.de, gkokolatos@protonmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: shared-memory based stats collector
In-Reply-To: <fa25c85a-2889-1ffc-84ff-2db2b2f7720e@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox