Thank you for looking this!
At Tue, 23 Jan 2024 15:07:10 +0900, Fujii Masao <masao.fujii@gmail.com> wrote in
> Regarding the patch, here are the review comments.
>
> +/*
> + * Is current process a wal receiver?
> + */
> +bool
> +IsWalReceiver(void)
> +{
> + return WalRcv != NULL;
> +}
>
> This looks wrong because WalRcv can be non-NULL in processes other
> than walreceiver.
Mmm. Sorry for the silly mistake. We can use B_WAL_RECEIVER
instead. I'm not sure if the new function IsWalReceiver() is
required. The expression "MyBackendType == B_WAL_RECEIVER" is quite
descriptive. However, the function does make ProcessInterrupts() more
aligned regarding process types.
> - pqsignal(SIGTERM, SignalHandlerForShutdownRequest); /* request shutdown */
> + pqsignal(SIGTERM, WalRcvShutdownSignalHandler); /* request shutdown */
>
> Can't we just use die(), instead?
There was a comment explaining the problems associated with exiting
within a signal handler;
- * Currently, only SIGTERM is of interest. We can't just exit(1) within the
- * SIGTERM signal handler, because the signal might arrive in the middle of
- * some critical operation, like while we're holding a spinlock. Instead, the
And I think we should keep the considerations it suggests. The patch
removes the comment itself, but it does so because it implements our
standard process exit procedure, which incorporates points suggested
by the now-removed comment.
--
Kyotaro Horiguchi
NTT Open Source Software Center