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: <20210312.092312.2023783779833776888.horikyota.ntt@gmail.com>
 <fa25c85a-2889-1ffc-84ff-2db2b2f7720e@oss.nttdata.com>
 <20210312.103800.78346667971630881.horikyota.ntt@gmail.com>
 <20210312.134906.647243209921851496.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
Message-ID: <7e633694-5702-e982-6c3f-0226189c8b41@oss.nttdata.com>
Date: Fri, 12 Mar 2021 15:13:15 +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.134906.647243209921851496.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/7e633694-5702-e982-6c3f-0226189c8b41%40oss.nttdata.com>
Precedence: bulk


On 2021/03/12 13:49, Kyotaro Horiguchi wrote:
> At Fri, 12 Mar 2021 10:38:00 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
>> At Fri, 12 Mar 2021 10:03:31 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in
>>>> 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?
>>
>> Although I intentinally didn't do that because "it really doesn't
>> matter", please wait for a while for the new version.  I'm going to
>> make PgArchData have only one member procno instead of replacing the
>> struct with a single variable.
> 
> 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()?

This is not fault of this patch. But last_pgarch_start_time should be initialized with zero?

+	if ((curtime - last_pgarch_start_time) < PGARCH_RESTART_INTERVAL)
+		return true;

Why did you remove the cast to unsigned int there?

+	/*
+	 * Advertise our latch that backends can use to wake us up while we're
+	 * sleeping.
+	 */
+	PgArch->pgprocno = MyProc->pgprocno;

The comment should be updated?

Regards,

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