Masahiko Sawada wrote:
> On Mon, Oct 22, 2018 at 3:01 PM Michael Paquier <michael@paquier.xyz> wrote:
> > Regarding the documentation, wouldn't it be more adapted to list the new
> > function under the section "Recovery Control Functions"? Not only does
> > the new function signal the postmaster, but it also creates the
> > promotion trigger file.
Hmm, yes, that's probably the first place where people would look.
I guess the implementation lead me to categorize it as a "signaling function",
and it also wouldn't be wrong there.
> > Anyway, I have been looking in depth at the patch, and I finish with the
> > attached. Here are some notes:
> > [...]
> > - WL_TIMEOUT could have been reached, leading to no further retries
> > (remove for example the part related to the trigger file creation and
> > try to trigger the promotion, the wait time is incorrect).
Ok.
> > - Documentation has been reworked.
> > - The new wait event is documented.
Thanks.
> > - position including unistd.h was not correct in xlogfuncs.c.
> > - Timeouts for the tests are made a tad longer. Some buildfarm machines
> > don't like a promotion wait of 10s.
> > - a catalog version bump is included, just a personal reminder..
> > - Indentatio has been run.
Thanks.
> Thank you for workig on this. There is one review comment for the latest patch.
>
> + if (FreeFile(promote_file))
> + ereport(ERROR,
> + (errcode_for_file_access(),
> + errmsg("could not write file \"%s\": %m",
> + PROMOTE_SIGNAL_FILE)));
>
> Maybe we should unlink PROMOTE_SIGNAL_FILE before erroring.
Yes, that cannot hurt.
Yours,
Laurenz Albe
