Re: Fix calculations on WAL recycling.

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

In response to

Browse pgsql-hackers by date

  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