> On 30 Aug 2021, at 12:31, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote:
> Here are some comments on the patch:
> 1) Do we need volatile keyword here to read the value of the variables
> always from the memory?
> +static volatile sig_atomic_t bgchild_exited = false;
Yes, fixed.
> 2) Do we need #ifndef WIN32 ... #endif around sigchld_handler function
> definition?
Ah yes, good point. Fixed.
> 3) I'm not sure if the new value of bgchild_exited being set in the
> child thread will reflect in the main process on Windows? But
> theoretically, I can understand that the memory will be shared between
> the main process thread and child thread.
The child does not have it’s own copy of bgchild_exited.
> 4) How about "set the same flag that we use on Unix to signal
> SIGCHLD." instead of "* set the flag used on Unix to signal
> SIGCHLD."?
Fixed.
> 5) How about "background WAL receiver terminated unexpectedly" instead
> of "log streamer child terminated unexpectedly"? This will be in sync
> with the existing message "starting background WAL receiver". "log
> streamer" is the word used internally in the code, user doesn't know
> it with that name.
Good point, that’s better.
> 6) How about giving the exit code (like postmaster's reaper function
> does) instead of just a message saying unexpected termination? It will
> be useful to know for what reason the process exited. For Windows, we
> can use GetExitCodeThread (I'm referring to the code around waitpid in
> pg_basebackup) and for Unix we can use waitpid.
The rest of the program is doing exit(1) regardless of the failure of the
child/thread, so it seems more consistent to keep doing that for this class of
error as well.
A v2 with the above fixes is attached.
--
Daniel Gustafsson https://vmware.com/
