On Mon, Sep 30, 2019 at 10:10 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Sep 27, 2019 at 10:00:16PM +0900, Masahiko Sawada wrote:
> > I abandoned once to move the removal code to between
> > writeTimeLineHistory() and timeline switching because of expanding the
> > window but since unlink itself will complete within a very short time
> > it would not be problamatic much.
> >
> > Attached the updated patch that just moves the removal code.
>
> That's not quite it, as you forgot to move the declaration of
> recoveryPath so the patch fails to compile.
Oops, thanks.
>
> Adding some tests would be nice, so I updated your patch to include
> something. One place where we recover files from archives is
> 002_archiving.pl, still the files get renamed to the segment names
> when recovered so that's difficult to make that part 100%
> deterministic yet. Still as a reminder of the properties behind those
> files it does not sound bad to document it in the test either, that's
> cheap, and we get the future covered.
Thank you for updating the patch!
+1 to add tests but even the current postgres passes this tests
because of two reasons: one is $node_standby tries to restore
00000001.history but fails and therefore RECOVERYHISTORY isn't
created. Another one is described To reproduce this issue the new
timeline ID of recovered database needs to be more than 3.
+isnt(
+ -f "$node_standby_data/pg_wal/RECOVERYHISTORY",
+ "RECOVERYHISTORY removed after promotion");
+isnt(
+ -f "$node_standby_data/pg_wal/RECOVERYXLOG",
+ "RECOVERYXLOG removed after promotion");
I think that the above checks are always true because isnt() function
checks if the 1st argument and 2nd argument are not the same.
I've attached the updated version patch including the tests. Please review it.
Regards,
--
Masahiko Sawada
