Message-ID: <01b8ecd8-7d95-1bff-7f8c-fac3d530aab7@oss.nttdata.com>
Date: Tue, 22 Feb 2022 01:59:45 +0900
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.6.0
Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
Content-Language: en-US
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: bharath.rupireddyforpostgres@gmail.com, nathandbossart@gmail.com,
 sfrost@snowman.net, bossartn@amazon.com, rjuju123@gmail.com,
 michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
References: <CALj2ACXkuMYfa1_RMtVyD+F8rg1F8fuPbku7wSyzdK79RvgdZw@mail.gmail.com>
 <20220209.115204.1794224638476710282.horikyota.ntt@gmail.com>
 <fa563db8-6f7d-ea8b-1ecf-69be07c099cc@oss.nttdata.com>
 <20220214.144022.2233054833762514893.horikyota.ntt@gmail.com>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <20220214.144022.2233054833762514893.horikyota.ntt@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/01b8ecd8-7d95-1bff-7f8c-fac3d530aab7%40oss.nttdata.com>
Precedence: bulk


On 2022/02/14 14:40, Kyotaro Horiguchi wrote:
> For backbranches, the attached for pg14 does part of the full patch.

Thanks for updating the patch!


> Of the following, I think we should do (a) and (b) to make future
> backpatchings easier.
> 
> a) Use RedoRecPtr and PriorRedoPtr after they are assigned.
> 
> b) Move assignment to PriorRedoPtr into the ControlFileLock section.

I failed to understand how (a) and (b) can make the backpatching easier. How easy to backpatch seems the same whether we apply (a) and (b) or not...


> c) Skip udpate of minRecoveryPoint only when the checkpoint gets old.

Yes.


> d) Skip call to UpdateCheckPointDistanceEstimate() when RedoRecPtr <=
>    PriorRedoPtr.

But "RedoRecPtr <= PriorRedoPtr" will never happen, will it? Because a restartpoint is skipped at the beginning of CreateRestartPoint() in that case. If this understanding is right, the check of "RedoRecPtr <= PriorRedoPtr" is not necessary before calling UpdateCheckPointDistanceEstimate().


+		ControlFile->minRecoveryPoint = InvalidXLogRecPtr;
+		ControlFile->minRecoveryPointTLI = 0;

Don't we need to update LocalMinRecoveryPoint and LocalMinRecoveryPointTLI after this? Maybe it's not necessary, but ISTM that it's safer and better to always update them whether the state is DB_IN_ARCHIVE_RECOVERY or not.


  		if (flags & CHECKPOINT_IS_SHUTDOWN)
  			ControlFile->state = DB_SHUTDOWNED_IN_RECOVERY;

Same as above. IMO it's safer and better to always update the state (whether the state is DB_IN_ARCHIVE_RECOVERY or not) if CHECKPOINT_IS_SHUTDOWN flag is passed.

Regards,

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