From: | Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> |
---|---|
To: | michael(at)paquier(dot)xyz |
Cc: | pgsql-hackers(at)postgresql(dot)org, hlinnaka(at)iki(dot)fi |
Subject: | Re: Fix calculations on WAL recycling. |
Date: | 2018-07-24 09:56:33 |
Message-ID: | 20180724.185633.180983749.horiguchi.kyotaro@lab.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
At Tue, 24 Jul 2018 10:41:18 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20180724014118(dot)GA4035(at)paquier(dot)xyz>
> On Mon, Jul 23, 2018 at 07:55:57PM +0900, Kyotaro HORIGUCHI wrote:
> 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.
Anyway good to hear that.
> >> 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.
Thank you for committing this.
I feel that XLOGfileslop() can be a function but I regret that I
didn't move it closer to RemoveOldXLogFiles a bit..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
From | Date | Subject | |
---|---|---|---|
Next Message | MyungKyu LIM | 2018-07-24 10:06:23 | RE: Re: [Proposal] Add accumulated statistics for wait event |
Previous Message | Kyotaro HORIGUCHI | 2018-07-24 09:27:16 | Re: BUG #15182: Canceling authentication due to timeout aka Denial of Service Attack |