Thread: [small patch] Change datatype of ParallelMessagePending from "volatile bool" to "volatile sig_atomic_t"

Hi hackers,

While reviewing [1], I and Amit noticed that a flag ParallelMessagePending is defined
as "volatile bool", but other flags set by signal handlers are defined as "volatile sig_atomic_t".

The datatype has been defined in standard C,
and it says that variables referred by signal handlers should be "volatile sig_atomic_t".
(Please see my observation [2])

This may be not needed because any failures had been reported,
but I thought their datatype should be same and attached a small patch.

How do you think?

[1]: https://commitfest.postgresql.org/39/3621/
[2]:
https://www.postgresql.org/message-id/TYAPR01MB5866C056BB9F81A42B85D20BF54E9%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Mon, Sep 26, 2022 at 06:57:28AM +0000, kuroda.hayato@fujitsu.com wrote:
> While reviewing [1], I and Amit noticed that a flag ParallelMessagePending is defined
> as "volatile bool", but other flags set by signal handlers are defined as "volatile sig_atomic_t".
>
> How do you think?

You are right.  bool is not usually a problem in a signal handler, but
sig_atomic_t is the type we ought to use.  I'll go adjust that.
--
Michael

On Mon, Sep 26, 2022 at 04:50:36PM +0900, Michael Paquier wrote:
> You are right.  bool is not usually a problem in a signal handler, but
> sig_atomic_t is the type we ought to use.  I'll go adjust that.

Done this one.  I have scanned the code, but did not notice a similar
mistake.  It is worth noting that we have only one remaining "volatile
bool" in the headers now.
--
Michael

Dear Michael,

> Done this one.  I have scanned the code, but did not notice a similar
> mistake.

I found your commit, thanks!

> It is worth noting that we have only one remaining "volatile
> bool" in the headers now.

Maybe you mentioned about sigint_interrupt_enabled,
and it also seems to be modified in the signal handler.
But I think any race conditions may be not occurred here
because if the value is set in the handler the code jump will be also happened.

Of course it's OK to mark the variable to sig_atomic_t too if there is no problem.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Tue, Sep 27, 2022 at 01:38:26AM +0000, kuroda.hayato@fujitsu.com wrote:
> Maybe you mentioned about sigint_interrupt_enabled,
> and it also seems to be modified in the signal handler.

Yeah, at least as of the cancel callback psql_cancel_callback() that
handle_sigint() would call on SIGINT as this is set by psql.  So it
does not seem right to use a boolean rather than a sig_atomic_t in
this case, as well.
--
Michael

Dear Michael,

> Yeah, at least as of the cancel callback psql_cancel_callback() that
> handle_sigint() would call on SIGINT as this is set by psql.  So it
> does not seem right to use a boolean rather than a sig_atomic_t in
> this case, as well.

PSA fix patch. Note that PromptInterruptContext.enabled was also fixed
because it is substituted from sigint_interrupt_enabled

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

On Wed, Sep 28, 2022 at 04:47:09AM +0000, kuroda.hayato@fujitsu.com wrote:
> PSA fix patch. Note that PromptInterruptContext.enabled was also fixed
> because it is substituted from sigint_interrupt_enabled.

Good point.  Thanks for the patch, this looks consistent!
--
Michael

On Wed, Sep 28, 2022 at 04:45:17PM +0900, Michael Paquier wrote:
> Good point.  Thanks for the patch, this looks consistent!

Done as of 5ac9e86.
--
Michael