Masahiko Sawada wrote: > On Mon, Oct 22, 2018 at 3:01 PM Michael Paquier <mich...@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