Re: Race condition in recovery?

On Sat, May 15, 2021 at 3:58 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> I did notice, but keep in mind that this was more than 8 years ago.
> Even if Heikki is reading this thread, he may not remember why he
> changed 1 line of code one way rather than another in 2013. I mean if
> he does that's great, but it's asking a lot.

I agree with your point. But I think that one line is related to the
purpose of this commit and I have explained (in 3rd paragraph below)
why do I think so.

As I understand it, the general issue here was that if
> XLogFileReadAnyTLI() was called before expectedTLEs got set, then
> prior to this commit it would have to fail, because the foreach() loop
> in that function would be iterating over an empty list. Heikki tried
> to make it not fail in that case, by setting tles =
> readTimeLineHistory(recoveryTargetTLI), so that the foreach loop
> *wouldn't* get an empty list.

I might be missing something but I don't agree with this logic. If
you see prior to this commit the code flow was like below[1]. So my
point is if we are calling XLogFileReadAnyTLI() somewhere while
reading the checkpoint record then note that expectedTLEs were
initialized unconditionally before even try to read that checkpoint
record. So how expectedTLEs could be uninitialized in
LogFileReadAnyTLI?

[1]
StartupXLOG(void)
{
....

recoveryTargetTLI = ControlFile->checkPointCopy.ThisTimeLineID;
...
readRecoveryCommandFile();
...
expectedTLEs = readTimeLineHistory(recoveryTargetTLI);
...
..
record = ReadCheckpointRecord(checkPointLoc, 0);

Another point which I am not sure about but still I think that one
line (expectedTLEs = readTimeLineHistory(receiveTLI);), somewhere
related to the purpose of this commit. Let me explain why do I think
so. Basically, before this commit, we were initializing
"expectedTLEs" based on the history file of "recoveryTargetTLI", right
after calling "readRecoveryCommandFile()" (this function will
initialize recoveryTargetTLI based on the recovery target, and it
ensures it read the respective history file). Now, right after this
point, there was a check as shown below[2], which is checking whether
the checkpoint TLI exists in the "expectedTLEs" which is initialized
based on recoveryTargetTLI. And it appeared that this check was
failing in some cases which this commit tried to fix and all other
code is there to support that. Because now before going for reading
the checkpoint we are not initializing "expectedTLEs" so now after
moving this line from here it was possible that "expectedTLEs" is not
initialized in XLogFileReadAnyTLI() and the remaining code in
XLogFileReadAnyTLI() is to handle that part.

Now, coming to my point that why this one line is related, In this
one line (expectedTLEs = readTimeLineHistory(receiveTLI);), we
completely avoiding recoveryTargetTLI and initializing "expectedTLEs"
based on the history file of the TL from which we read the checkpoint,
so now, there is no scope of below[2] check to fail because note that
we are not initializing "expectedTLEs" based on the
"recoveryTargetTLI" but we are initializing from the history from
where we read checkpoint.

So I feel if we directly fix this one line to initialize
"expectedTLEs" from "recoveryTargetTLI" then it will expose to the
same problem this commit tried to fix.

[2]
if (tliOfPointInHistory(ControlFile->checkPoint, expectedTLEs) !=
ControlFile->checkPointCopy.ThisTimeLineID)
{
error()
}

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com