Subject: Re: shared-memory based stats collector
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: andres@anarazel.de, gkokolatos@protonmail.com,
 pgsql-hackers@lists.postgresql.org
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>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <fa25c85a-2889-1ffc-84ff-2db2b2f7720e@oss.nttdata.com>
Date: Fri, 12 Mar 2021 10:03:31 +0900
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0)
 Gecko/20100101 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <20210312.092312.2023783779833776888.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/fa25c85a-2889-1ffc-84ff-2db2b2f7720e%40oss.nttdata.com>
Precedence: bulk


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