Re: Race condition in recovery? - Mailing list pgsql-hackers

From Kyotaro Horiguchi
Subject Re: Race condition in recovery?
Date
Msg-id 20210608.102918.2136451872499052401.horikyota.ntt@gmail.com
Whole thread Raw
In response to Re: Race condition in recovery?  (Robert Haas <robertmhaas@gmail.com>)
List pgsql-hackers
At Mon, 7 Jun 2021 10:40:27 -0400, Robert Haas <robertmhaas@gmail.com> wrote in 
> On Mon, Jun 7, 2021 at 12:57 AM Kyotaro Horiguchi
> <horikyota.ntt@gmail.com> wrote:
> > Unfortunately no. The backslashes in the binary path need to be
> > escaped. (taken from PostgresNode.pm:1008)
> >
> > > (my $perlbin = $^X) =~ s{\\}{\\\\}g if ($TestLib::windows_os);
> > > $node_primary->append_conf(
> > >       'postgresql.conf', qq(
> > > archive_command = '$perlbin "$FindBin::RealBin/cp_history_files" "%p" "$archivedir_primary/%f"'
> > > ));
> >
> > This works for me.
> 
> Hmm, OK. Do you think we also need to use perl2host in this case?

I understand that perl2host converts '/some/where' style path to the
native windows path 'X:/any/where' if needed. Since perl's $^X is
already in native style so I think we don't need to use it.

> > Ugh! Sorry. I meant "The explicit teardowns are useless". That's not
> > harmful but it is done by PostgresNode.pm automatically(implicitly)
> > and we don't do that in the existing scripts.
> 
> OK. I don't think it's a big deal, but we can remove them.

Thanks.

> I went back and looked at your patch again, now that I understand the
> issue better. I believe it's not necessary to do this here, because
> StartupXLOG() already contains a check for the same thing:
> 
>     /*
>      * If the location of the checkpoint record is not on the expected
>      * timeline in the history of the requested timeline, we cannot proceed:
>      * the backup is not part of the history of the requested timeline.
>      */
>     Assert(expectedTLEs);       /* was initialized by reading checkpoint
>                                  * record */
>     if (tliOfPointInHistory(checkPointLoc, expectedTLEs) !=
>         checkPoint.ThisTimeLineID)
> ...
> 
> This code is always run after ReadCheckpointRecord() returns. And I
> think that your only concern here is about the case where the
> checkpoint record is being fetched, because otherwise expectedTLEs
> must already be set.

Sure. Thanks for confirming that, and agreed.

> By the way, I also noticed that your version of the patch contains a
> few words which are spelled incorrectly: hearafter, and incosistent.

Mmm. Sorry for them..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



pgsql-hackers by date:

Previous
From: Kyotaro Horiguchi
Date:
Subject: Re: Logical replication keepalive flood
Next
From: Julien Rouhaud
Date:
Subject: Re: SQL-standard function body