Message-ID: <2adc5283-ae2c-424a-babf-0247eeebf5a3@oss.nttdata.com>
Date: Mon, 30 Sep 2024 12:26:56 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: May be BUG. Periodic burst growth of the checkpoint_req counter
 on replica.
Content-Language: en-US
To: "Anton A. Melnikov" <a.melnikov@postgrespro.ru>,
 Alexander Korotkov <aekorotkov@gmail.com>
Cc: Magnus Hagander <magnus@hagander.net>,
 "Anton A. Melnikov" <aamelnikov@inbox.ru>, Andres Freund
 <andres@anarazel.de>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 pgsql-hackers@lists.postgresql.org
References: <99b2ccd1-a77a-962a-0837-191cdf56c2b9@inbox.ru>
 <7bd1c12b-4219-45bf-88a6-8cacd008dfdc@postgrespro.ru>
 <CAPpHfduW_LbrU1JPtEDek5Fj3_=GZRU=qvFMyg7hnky7BLCGVw@mail.gmail.com>
 <CABUevExav5-SR0x+G9kBUMV0G8XsvSUfuyyqmYBBJi6VHns6sw@mail.gmail.com>
 <CAPpHfdtfDPEy4Pf1n2LvhsQ8ZsE40=OcFf=+U3BAQvfCtdj0MQ@mail.gmail.com>
 <3ed0590b-8132-482e-a9e6-bd694e4a48d7@postgrespro.ru>
 <CAPpHfduFpdq0dUsF=SmTtYit684JoSQrXvzhA+hap-tCA8Z2cA@mail.gmail.com>
 <CAPpHfdss-+EZHUP_MMNm9wHm43+ZM84ERWZvGO+p-5kH_Qt6og@mail.gmail.com>
 <9ea77f40-818d-4841-9dee-158ac8f6e690@oss.nttdata.com>
 <77032579-4dc3-4552-9a09-30aaa114c144@postgrespro.ru>
 <206e58c1-d8f6-4ac3-8f40-7e4c17459473@oss.nttdata.com>
 <a9cbe052-7cd2-46d2-a688-c7b08919db83@postgrespro.ru>
 <82f03b0b-00c6-42b4-95c7-f2eca7fed1ea@oss.nttdata.com>
 <a7f75d42-3a2e-4d70-a1b1-27c8706d2897@postgrespro.ru>
 <4640258e-d959-4cf0-903c-cd02389c3e05@oss.nttdata.com>
 <8e5f353f-8b31-4a8e-9cfa-c037f22b4aee@postgrespro.ru>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <8e5f353f-8b31-4a8e-9cfa-c037f22b4aee@postgrespro.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: <https://www.postgresql.org/message-id/2adc5283-ae2c-424a-babf-0247eeebf5a3%40oss.nttdata.com>
Precedence: bulk


On 2024/09/22 13:55, Anton A. Melnikov wrote:
> On 20.09.2024 19:19, Fujii Masao wrote:
>> I've attached the updated version (0001.patch). I made some cosmetic changes,
>> including reverting the switch in the entries for pg_stat_get_checkpointer_write_time
>> and pg_stat_get_checkpointer_sync_time in pg_proc.dat, as I didn’t think
>> that change was necessary. Could you please review the latest version?
> 
> Thanks for corrections!
> All looks good for me.

Thanks for the review! I've pushed the 0001 patch.


> As for switching in the pg_proc.dat entries the idea was to put them in order
> so that the pg_stat_get_checkpointer* functions were grouped together.
> I don't know if this is the common and accepted practice. Simply i like it better this way.
> Sure, if you think it's unnecessary, let it stay as is with minimal diff.

I understand your point, but I didn't made that change to keep the diff minimal,
which should make future back-patching easier.


>> After we commit 0001.patch, how about applying 0002.patch, which updates
>> the documentation for the pg_stat_checkpointer view to clarify what types
>> of checkpoints and restartpoints each counter tracks?
> 
> I liked that the short definitions of the counters are now separated from
> the description of its work features which are combined into one paragraph.
> It seems to me that is much more logical and easier to understand.

Thanks for the review!


> In addition, checkpoints may be skipped due to "checkpoints are occurring
> too frequently" error. Not sure, but maybe add this information to
> the new description?

 From what I can see in the code, that error message doesn’t seem to indicate
the checkpoint is being skipped. In fact, checkpoints are still happening
actually when that message appears. Am I misunderstanding something?


>> In 0002.patch, I also modified the description of num_requested from
>> "Number of backend requested checkpoints" to remove "backend," as it can
>> be confusing since num_requested includes requests from sources other than
>> the backend. Thought?
> 
> Agreed. E.g. from xlog. Then maybe changed it also in the function
> descriptions in the pg_proc.dat? For pg_stat_get_checkpointer_num_requested()
> and pg_stat_get_checkpointer_restartpoints_requested().

Yes, good catch!

Regards,

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