Thread: Correct comment in RemoveNonParentXlogFiles()
HI All,
Following comment in RemoveNonParentXlogFiles() says that we are trying to remove any WAL file whose segment number is >= the segment number of the first WAL file on the new timeline. However, looking at the code, I can say that we are trying to remove the WAL files from the previous timeline whose segment number is just greater than (not equal to) the segment number of the first WAL file in the new timeline. I think we should improve this comment, thoughts?
/*
* Remove files that are on a timeline older than the new one we're
* switching to, but with a segment number >= the first segment on the
* new timeline.
*/
if (strncmp(xlde->d_name, switchseg, 8) < 0 &&
strcmp(xlde->d_name + 8, switchseg + 8) > 0)
/*
* Remove files that are on a timeline older than the new one we're
* switching to, but with a segment number >= the first segment on the
* new timeline.
*/
if (strncmp(xlde->d_name, switchseg, 8) < 0 &&
strcmp(xlde->d_name + 8, switchseg + 8) > 0)
--
With Regards,
Ashutosh Sharma.
At Wed, 3 Aug 2022 18:16:33 +0530, Ashutosh Sharma <ashu.coek88@gmail.com> wrote in > Following comment in RemoveNonParentXlogFiles() says that we are trying to > remove any WAL file whose segment number is >= the segment number of the > first WAL file on the new timeline. However, looking at the code, I can say > that we are trying to remove the WAL files from the previous timeline whose > segment number is just greater than (not equal to) the segment number of > the first WAL file in the new timeline. I think we should improve this > comment, thoughts? > > /* > * Remove files that are on a timeline older than the new one we're > * switching to, but with a segment number >= the first segment on > the > * new timeline. > */ > if (strncmp(xlde->d_name, switchseg, 8) < 0 && > strcmp(xlde->d_name + 8, switchseg + 8) > 0) I'm not sure I'm fully getting your point. The current comment is correctly saying that it removes the segments "on a timeline older than the new one". I agree about segment comparison. So, if I changed that comment, I would finish with the following change. - * switching to, but with a segment number >= the first segment on + * switching to, but with a segment number greater than the first segment on That disagreement started at the time the code was introduced by b2a5545bd6. Leaving the last segment in the old timeline is correct since it is renamed to .partial later. If timeline switch happened just at segment boundary, that segment would not not be created. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
At Thu, 04 Aug 2022 15:00:06 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in > At Wed, 3 Aug 2022 18:16:33 +0530, Ashutosh Sharma <ashu.coek88@gmail.com> wrote in > > Following comment in RemoveNonParentXlogFiles() says that we are trying to > > remove any WAL file whose segment number is >= the segment number of the > > first WAL file on the new timeline. However, looking at the code, I can say > > that we are trying to remove the WAL files from the previous timeline whose > > segment number is just greater than (not equal to) the segment number of > > the first WAL file in the new timeline. I think we should improve this > > comment, thoughts? > > > > /* > > * Remove files that are on a timeline older than the new one we're > > * switching to, but with a segment number >= the first segment on > > the > > * new timeline. > > */ > > if (strncmp(xlde->d_name, switchseg, 8) < 0 && > > strcmp(xlde->d_name + 8, switchseg + 8) > 0) > > I'm not sure I'm fully getting your point. The current comment is > correctly saying that it removes the segments "on a timeline older > than the new one". I agree about segment comparison. > > So, if I changed that comment, I would finish with the following change. > > - * switching to, but with a segment number >= the first segment on > + * switching to, but with a segment number greater than the first segment on > > That disagreement started at the time the code was introduced by > b2a5545bd6. Leaving the last segment in the old timeline is correct > since it is renamed to .partial later. If timeline switch happened > just at segment boundary, that segment would not not be created. "the last segment in the old timeline" here means "the segment in the old timeline, with the segment number == the first segmetn on the new timeline". regards. -- Kyotaro Horiguchi NTT Open Source Software Center
On Thu, Aug 4, 2022 at 11:30 AM Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote:
At Wed, 3 Aug 2022 18:16:33 +0530, Ashutosh Sharma <ashu.coek88@gmail.com> wrote in
> Following comment in RemoveNonParentXlogFiles() says that we are trying to
> remove any WAL file whose segment number is >= the segment number of the
> first WAL file on the new timeline. However, looking at the code, I can say
> that we are trying to remove the WAL files from the previous timeline whose
> segment number is just greater than (not equal to) the segment number of
> the first WAL file in the new timeline. I think we should improve this
> comment, thoughts?
>
> /*
> * Remove files that are on a timeline older than the new one we're
> * switching to, but with a segment number >= the first segment on
> the
> * new timeline.
> */
> if (strncmp(xlde->d_name, switchseg, 8) < 0 &&
> strcmp(xlde->d_name + 8, switchseg + 8) > 0)
I'm not sure I'm fully getting your point. The current comment is
correctly saying that it removes the segments "on a timeline older
than the new one". I agree about segment comparison.
Yeah my complaint is about the comment on segment comparison for removal.
So, if I changed that comment, I would finish with the following change.
- * switching to, but with a segment number >= the first segment on
+ * switching to, but with a segment number greater than the first segment on
which looks correct to me and will inline it with the code.
That disagreement started at the time the code was introduced by
b2a5545bd6. Leaving the last segment in the old timeline is correct
since it is renamed to .partial later. If timeline switch happened
just at segment boundary, that segment would not not be created.
Yeah, that's why we keep the last segment (partially written) from the old timeline, which means we're not deleting it here. So the comment should not be saying that we are also removing the last wal segment from the old timeline which is equal to the first segment from the new timeline.
--
With Regards,
Ashutosh Sharma.