MIME-Version: 1.0
References: <20220207.120258.310426179780547983.horikyota.ntt@gmail.com>
 <b3b23cb7-9002-d6a3-2189-fa9ca99b0969@oss.nttdata.com> <20220208.165822.2155503452120234158.horikyota.ntt@gmail.com>
 <20220208.174029.1890340465677956009.horikyota.ntt@gmail.com>
In-Reply-To: <20220208.174029.1890340465677956009.horikyota.ntt@gmail.com>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Tue, 8 Feb 2022 14:33:01 +0530
Message-ID: <CALj2ACXkuMYfa1_RMtVyD+F8rg1F8fuPbku7wSyzdK79RvgdZw@mail.gmail.com>
Subject: Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Fujii Masao <masao.fujii@oss.nttdata.com>, Nathan Bossart <nathandbossart@gmail.com>, 
	Stephen Frost <sfrost@snowman.net>, "Bossart, Nathan" <bossartn@amazon.com>, 
	Julien Rouhaud <rjuju123@gmail.com>, Michael Paquier <michael@paquier.xyz>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CALj2ACXkuMYfa1_RMtVyD%2BF8rg1F8fuPbku7wSyzdK79RvgdZw%40mail.gmail.com>
Precedence: bulk

On Tue, Feb 8, 2022 at 2:10 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
> > Thus.. the attached removes the ambiguity of of the proposed patch
> > about the LSNs in the restartpoint-ending log message.
>
> Thoughts?

Thanks for the patch. I have few comments on the
v1-0001-Get-rid-of-unused-path-to-handle-concurrent-check.patch

1) Can we have this Assert right after "skipping restartpoint, already
performed at %X/%X" error message block? Does it make any difference?
My point is that if at all, we were to assert this, why can't we do it
before CheckPointGuts?
+ /* We mustn't have a concurrent checkpoint that advances checkpoint LSN */
+ Assert(lastCheckPoint.redo > ControlFile->checkPointCopy.redo);
+
2) Related to the above Assert, do we really need an assertion or a FATAL error?
3) Let's be consistent with "crash recovery" - replace
"archive-recovery" with "archive recovery"?
+ * We have exited from archive-recovery mode after this restartpoint
+ * started. Crash recovery ever after should always recover to the end
4) Isn't it enough to say "Crash recovery should always recover to the
end of WAL."?
+ * started. Crash recovery ever after should always recover to the end
5) Is there a reliable test case covering this code? Please point me
if the test case is shared upthread somewhere.
6) So, with this patch, the v8 patch-set posted at [1] doesn't need
any changes IIUC. If that's the case, please feel free to post all the
patches together such that they get tested in cfbot.

[1] - https://www.postgresql.org/message-id/CALj2ACUtZhTb%3D2ENkF3BQ3wi137uaGi__qzvXC-qFYC0XwjALw%40mail.gmail.com

Regards,
Bharath Rupireddy.