Horton

Re: Race condition in recovery?

Dilip Kumar <dilipbalaut@gmail.com>

View thread View on postgr.es
From: Dilip Kumar <dilipbalaut@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Kyotaro Horiguchi <horikyota.ntt@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-05-21T14:39:20Z
Lists: pgsql-hackers 
On Thu, May 20, 2021 at 11:19 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Tue, May 18, 2021 at 1:33 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > Yeah, it will be a fake 1-element list.  But just to be clear that
> > 1-element can only be "ControlFile->checkPointCopy.ThisTimeLineID" and
> > nothing else, do you agree to this?  Because we initialize
> > recoveryTargetTLI to this value and we might change it in
> > readRecoveryCommandFile() but for that, we must get the history file,
> > so if we are talking about the case where we don't have the history
> > file then "recoveryTargetTLI" will still be
> > "ControlFile->checkPointCopy.ThisTimeLineID".
>
> I don't think your conclusion is correct. As I understand it, you're
> talking about the state before
> ee994272ca50f70b53074f0febaec97e28f83c4e,

Right, I am talking about before this commit.

 because as of now
> readRecoveryCommandFile() no longer exists in master. Before that
> commit, StartupXLOG did this:
>
>         recoveryTargetTLI = ControlFile->checkPointCopy.ThisTimeLineID;
>         readRecoveryCommandFile();
>         expectedTLEs = readTimeLineHistory(recoveryTargetTLI);
>
> Now, readRecoveryCommandFile() can change recoveryTargetTLI. Before
> doing so, it will call existsTimeLineHistory if
> recovery_target_timeline was set to an integer, and findNewestTimeLine
> if it was set to latest. In the first case, existsTimeLineHistory()
> calls RestoreArchivedFile(), but that restores it using a temporary
> name, and KeepFileRestoredFromArchive is not called,

Yes, I agree with this.

so we might have
> the timeline history in RECOVERYHISTORY but that's not the filename
> we're actually going to try to read from inside readTimeLineHistory().
> In the second case, findNewestTimeLine() will call
> existsTimeLineHistory() which results in the same situation. So I
> think when readRecoveryCommandFile() returns expectedTLI can be
> different but the history file can be absent since it was only ever
> restored under a temporary name.

I agree that readTimeLineHistory() will not look for that filename,
but it will also try to get the file using (RestoreArchivedFile(path,
histfname, "RECOVERYHISTORY", 0, false)).  So after we check the
history file existence in existsTimeLineHistory(), if the file got
removed from the archive (not sure how) then it is possible that now
readTimeLineHistory() will not find that history file again.  Am I
missing something?

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

Commits

  1. Further refinement of stuck_on_old_timeline recovery test

  2. Adjust new test case to set wal_keep_segments.

  3. Adjust new test case to set wal_keep_size.

  4. Fix corner case failure of new standby to follow new primary.

  5. Back-port a few PostgresNode.pm methods.

  6. Allow PostgresNode.pm's backup method to accept backup_options.

  7. Add end-to-end testing of pg_basebackup's tar-format output.

  8. Allow PostgresNode.pm tests to wait for catchup

  9. Delay reading timeline history file until it's fetched from master.