On Sat, May 15, 2021 at 3:58 AM Robert Haas <robertmh...@gmail.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