Re: Race condition in recovery?

Thanks.

At Fri, 7 May 2021 11:04:53 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> On Fri, May 7, 2021 at 8:23 AM Kyotaro Horiguchi
> <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> >
> > At Tue, 4 May 2021 17:41:06 +0530, Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote in
> > Could you please fix the test script so that it causes your issue
> > correctly? And/or elaborate a bit more?
> >
> > The attached first file is the debugging aid logging. The second is
> > the test script, to be placed in src/test/recovery/t.
>
> I will look into your test case and try to see whether we can
> reproduce the issue. But let me summarise what is the exact issue.
> Basically, the issue is that first in validateRecoveryParameters if
> the recovery target is the latest then we fetch the latest history
> file and set the recoveryTargetTLI timeline to the latest available
> timeline assume it's 2 but we delay updating the expectedTLEs (as per
> commit ee994272ca50f70b53074f0febaec97e28f83c4e). Now, while reading

I think it is right up to here.

> the checkpoint record if we don't get the required WAL from the
> archive then we try to get from primary, and while getting checkpoint
> from primary we use "ControlFile->checkPointCopy.ThisTimeLineID"
> suppose that is older timeline 1. Now after reading the checkpoint we
> will set the expectedTLEs based on the timeline from which we got the
> checkpoint record.

I doubt this point. ReadCheckpointRecord finally calls
XLogFileReadAnyTLI and it uses the content of the 00000002.history as
the local timeline entry list, since expectedTLEs is NIL and
recoveryTargetTLI has been updated to 2 by
validateRecoveryParameters(). But node 3 was having only the segment
on TLI=1 so ReadCheckPointRecord() finds the wanted checkpoint recrod
on TLI=1. XLogFileReadAnyTLI() copies the local TLE list based on
TLI=2 to expectedTLEs just after that because the wanted checkpoint
record was available based on the list.

So I don't think checkPointCopy.ThisTimeLineID cannot affect this
logic, and don't think expectedTLEs is left with NIL. It's helpful
that you could show the specific code path to cause that.

> See below Logic in WaitForWalToBecomeAvailable
> if (readFile < 0)
> {
> if (!expectedTLEs)
> expectedTLEs = readTimeLineHistory(receiveTLI);
>
> Now, the first problem is we are breaking the sanity of expectedTLEs
> because as per the definition it should already start with
> recoveryTargetTLI but it is starting with the older TLI. Now, in

If my description above is correct, expectedTLEs has been always
filled by TLI=2's hisotry so readTimeLineHistory is not called there.

After that the things are working as described in my previous mail. So
The following is not an issue if I'm not missing something.

> rescanLatestTimeLine we are trying to fetch the latest TLI which is
> still 2, so this logic returns without reinitializing the expectedTLEs
> because it assumes that if recoveryTargetTLI is pointing to 2 then
> expectedTLEs must be correct and need not be changed.
>
> See below logic:
> rescanLatestTimeLine(void)
> {
> ....
> newtarget = findNewestTimeLine(recoveryTargetTLI);
> if (newtarget == recoveryTargetTLI)
> {
> /* No new timelines found */
> return false;
> }
> ...
> newExpectedTLEs = readTimeLineHistory(newtarget);
> ...
> expectedTLEs = newExpectedTLEs;
>
>
> Solution:
> 1. Find better way to fix the problem of commit
> (ee994272ca50f70b53074f0febaec97e28f83c4e) which is breaking the
> sanity of expectedTLEs.
> 2. Assume, we have to live with fix 1 and we have to initialize
> expectedTLEs with an older timeline for validating the checkpoint in
> absence of tl.hostory file (as this commit claims). Then as soon as
> we read and validate the checkpoint, fix the expectedTLEs and set it
> based on the history file of recoveryTargetTLI.
>
> Does this explanation make sense? If not please let me know what part
> is not clear in the explanation so I can point to that code.

So, unfortunately not.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center