On Wed, Aug 30, 2023 at 1:03 PM Michael Paquier <mich...@paquier.xyz> wrote: > > On Mon, Aug 28, 2023 at 01:47:18PM +0530, Dilip Kumar wrote: > > I removed this mainly because now in other comments[1] where we are > > introducing this new CHECKPOINT_REDO record we are explaining the > > problem > > that the redo location and the actual checkpoint records are not at > > the same place and that is because of the concurrent xlog insertion. > > I think we are explaining in more > > detail by also stating that in case of a shutdown checkpoint, there > > would not be any concurrent insertion so the shutdown checkpoint redo > > will be at the same place. So I feel keeping old comments is not > > required. > > And we are explaining it when we are setting this for > > non-shutdown checkpoint because for shutdown checkpoint this statement > > is anyway not correct because for the shutdown checkpoint the > > checkpoint record will be at the same location and there will be no > > concurrent wal insertion, what do you think? > > + * Insert a dummy CHECKPOINT_REDO record and set start LSN of this record > + * as checkpoint.redo. > > I would add a "for a non-shutdown checkpoint" at the end of this > sentence. > > + * record. So when processing the wal, we cannot rely on the checkpoint > + * record if we want to stop at the checkpoint-redo LSN. > > The term "checkpoint-redo" is also a bit confusing, I guess, because > you just mean to refer to the "redo" LSN here? Maybe rework the last > sentence as: > "So, when processing WAL, we cannot rely on the checkpoint record if > we want to stop at the same position as the redo LSN". > > + * This special record, however, is not required when we are doing a > + * shutdown checkpoint, as there will be no concurrent wal insertions > + * during that time. So, the shutdown checkpoint LSN will be the same as > + * checkpoint-redo LSN. > > Perhaps the last sentence could be merged with the first one, if we > are tweaking things, say: > "This special record is not required when doing a shutdown checkpoint; > the redo LSN is the same LSN as the checkpoint record as there cannot > be any WAL activity in a shutdown sequence."
Your suggestions LGTM so modified accordingly -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
v5-0001-New-WAL-record-for-checkpoint-redo-location.patch
Description: Binary data