On Tue, Feb 8, 2022 at 2:10 PM Kyotaro Horiguchi <horikyota....@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.
