Re: Fix calculations on WAL recycling.

On Mon, Jul 23, 2018 at 07:55:57PM +0900, Kyotaro HORIGUCHI wrote:
> At Mon, 23 Jul 2018 15:59:16 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180723065916(dot)GI2854(at)paquier(dot)xyz>
>> This is an open item for v11.
>
> Mmm. Thanks. I wrongly thought this was v10 item. Removed this
> from the next CF.

Thanks for updating the CF app.

>> By doing so, you would notice that the oldest WAL segment does not get
>> recycled after the checkpoint, while it should as the redo pointer is
>> always checkpoint generated. What happens is that this oldest segment
>> gets recycled every two checkpoints.
>
> (I'm not sure I'm getting it correctly..) I see the old segments
> recycled. When I see pg_switch_wal() returns 0/12..,
> pg_walfile_name is ...13 and segment files for 13 and 14 are
> found in pg_wal directory. That is, seg 12 was recycled as seg
> 14. log_checkpoint=on shows every checkpoint recycles 1 segment
> in the case.

With your patch applied I see one segment recycled after each
checkpoint, which is correct. On HEAD, you would see no segments,
followed by 2 segments recycled. But I also see sometimes one segment
recycled.

>> The calculation of _logSegNo in CreateRestartPoint is visibly incorrect,
>> this still uses PriorRedoPtr so the bug is not fixed for standbys. The
>> tweaks for ThisTimeLineID also need to be out of the loop where
>> PriorRedoPtr is InvalidXLogRecPtr, and only the distance calculation
>> should be kept.
>
> Agreed. While I reconsidered this, I noticed that the estimated
> checkpoint distance is 0 when PriorRedoPtr is invalid. This means
> that the first checkpoint/restartpoint tries to reduce WAL
> segments to min_wal_size. However, it happens only at initdb time
> and makes no substantial behavior change so I made the change
> ignoring the difference.

Yes, same analysis here.

>> 1) and 2) are in my opinion clearly oversights from 4b0d28d, but 3) is
>> not.
>
> Thank you for the comments and suggestions. After all, I did the
> following things in the attached patch.
>
> - Reverted the change on timeline switching. (Removed the (3))
> - Fixed CreateRestartPoint to do the same thing with CreateCheckPoint.
> - Both CreateRestart/CheckPoint now tries trimming of WAL
> segments even for the first time.

Thanks, pushed after some comment tweaks and fixing the variable names
at the top of xlog.c for the static declarations. Perhaps we can do
more refactoring here by moving all the segment calculation logic
directly in RemoveOldXlogFiles, but that makes the end LSN calculation a
bit blurry so I kept things as you proposed in version 3 of the patch.
--
Michael