Re: Should we remove a fallback promotion? take 2

On 2020/06/03 3:38, Hamid Akhtar wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, passed
> Implements feature: not tested
> Spec compliant: not tested
> Documentation: not tested
>
> I've applied the v2 patch on the master branch. There some hunks, but the patch got applied. So, I ran make installcheck-world and everything looks fine to me with this patch. Though, I do have a few suggestions in general:

Thanks for the test and review!

> (1) I see two functions being used (a) CheckPromoteSignal and (b) IsPromoteSignaled in the code. Should these be combined into a single function and perhaps check for "promote_signaled" and the "PROMOTE_SIGNAL_FILE". Not sure if doing this will break "sigusr1_handler" in postmaster.c though.

I don't think we can do that simply. CheckPromoteSignal() can be called by
both postmaster and the startup process. OTOH, IsPromoteSignaled()
accesses the flag that can be set only in the startup process' signal handler,
i.e., it's intended to be called only by the startup process.

> (2) CheckPromoteSignal is checking for "PROMOTE_SIGNAL_FILE" file. So, perhaps, rather than calling stat on "PROMOTE_SIGNAL_FILE" in if statements, I would suggest to use CheckPromoteSignal function instead as it does nothing but stat on "PROMOTE_SIGNAL_FILE" (after applying your patch).

Yes, that's good idea. Attached is the updated version of the patch.
I replaced that stat() with CheckPromoteSignal(). Also I replaced
unlink(PROMOTE_SIGNAL_FILE) with RemovePromoteSignalFiles().

> The new status of this patch is: Waiting on Author

I will change the status back to Needs Review.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION