Thanks for your review! 2023年9月27日(水) 8:33 Michael Paquier <mich...@paquier.xyz>:
> On Tue, Sep 26, 2023 at 06:44:50PM +0300, Aleksander Alekseev wrote:
> >> And also, I'm afraid that I'm not sure what kind of tests I have to make
> >> for fix this behavior. Would you mind giving me some advice?
> >
> > Personally I would prefer not to increase the scope of work. Your TAP
> > test added in 0001 seems to be adequate.
>
> Yeah, agreed. I'm OK with what you are proposing, basically (the
> test could be made a bit cheaper actually).
I guess you meant that it contains an unnecessary insert and wait.
I fixed this and some incorrect comments caused by copy & paste.
Please see the attached patch.
>
/*
> - * For Hot Standby, we could treat this like a Shutdown
> Checkpoint,
> - * but this case is rarer and harder to test, so the benefit
> doesn't
> - * outweigh the potential extra cost of maintenance.
> + * For Hot Standby, we could treat this like an end-of-recovery
> checkpoint
> */
> + RequestCheckpoint(CHECKPOINT_IMMEDIATE | CHECKPOINT_WAIT);
>
> I don't understand what you want to change here. Archive recovery and
> crash recovery are two different things, still this code would be
> triggered even if there is no standby.signal, aka the node is not a
> standby. Why isn't this stuff conditional?
You are absolutely right. It should only run in standby mode.
Also, according to the document[1], a server can be "Hot Standby" even if
it is
not in standby mode (i.e. when it is in archive recovery mode).
So I fixed the comment above `RequestCheckpoint()`.
[1]: https://www.postgresql.org/docs/current/hot-standby.html
I hope you will review it again.
Regards,
Masaki Kuwamura
v3-0001-pg_rewind-Fix-bug-using-cascade-standby-as-source.patch
Description: Binary data
