[small patch] Change datatype of ParallelMessagePending from "volatile bool" to "volatile sig_atomic_t"
Hi hackers,
While reviewing [1]https://commitfest.postgresql.org/39/3621/, 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]/messages/by-id/TYAPR01MB5866C056BB9F81A42B85D20BF54E9@TYAPR01MB5866.jpnprd01.prod.outlook.com)
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]: /messages/by-id/TYAPR01MB5866C056BB9F81A42B85D20BF54E9@TYAPR01MB5866.jpnprd01.prod.outlook.com
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
0001-Change-datatype-of-ParallelMessagePending-to-keep-co.patchapplication/octet-stream; name=0001-Change-datatype-of-ParallelMessagePending-to-keep-co.patchDownload
From b39a3d13a2170d770be6677d1135b3b43ed12b65 Mon Sep 17 00:00:00 2001
From: "kuroda.hayato%40jp.fujitsu.com" <kuroda.hayato@jp.fujitsu.com>
Date: Mon, 26 Sep 2022 06:18:25 +0000
Subject: [PATCH] Change datatype of ParallelMessagePending to keep consistency
with other codes
The flag ParallelMessagePending is defined as bool from initial commit,
but any other flags set by signal handlers are defined as sig_atomic_t.
This commit fixes it based on other ones.
---
src/backend/access/transam/parallel.c | 2 +-
src/include/access/parallel.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/transam/parallel.c b/src/backend/access/transam/parallel.c
index bc93101ff7..8cba888223 100644
--- a/src/backend/access/transam/parallel.c
+++ b/src/backend/access/transam/parallel.c
@@ -113,7 +113,7 @@ typedef struct FixedParallelState
int ParallelWorkerNumber = -1;
/* Is there a parallel message pending which we need to receive? */
-volatile bool ParallelMessagePending = false;
+volatile sig_atomic_t ParallelMessagePending = false;
/* Are we initializing a parallel worker? */
bool InitializingParallelWorker = false;
diff --git a/src/include/access/parallel.h b/src/include/access/parallel.h
index 983841d45e..1ec8e33af4 100644
--- a/src/include/access/parallel.h
+++ b/src/include/access/parallel.h
@@ -54,7 +54,7 @@ typedef struct ParallelWorkerContext
shm_toc *toc;
} ParallelWorkerContext;
-extern PGDLLIMPORT volatile bool ParallelMessagePending;
+extern PGDLLIMPORT volatile sig_atomic_t ParallelMessagePending;
extern PGDLLIMPORT int ParallelWorkerNumber;
extern PGDLLIMPORT bool InitializingParallelWorker;
--
2.27.0
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
Attachments:
0001-Mark-sigint_interrupt_enabled-as-sig_atomic_t.patchapplication/octet-stream; name=0001-Mark-sigint_interrupt_enabled-as-sig_atomic_t.patchDownload
From 4fe60b47b97626ca207525ef557ec3f8f124a001 Mon Sep 17 00:00:00 2001
From: "kuroda.hayato%40jp.fujitsu.com" <kuroda.hayato@jp.fujitsu.com>
Date: Wed, 28 Sep 2022 04:33:27 +0000
Subject: [PATCH] Mark sigint_interrupt_enabled as sig_atomic_t
---
src/bin/psql/common.c | 2 +-
src/bin/psql/common.h | 3 ++-
src/include/common/string.h | 4 +++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index e611e3266d..4f42676066 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -246,7 +246,7 @@ NoticeProcessor(void *arg, const char *message)
* On Windows, currently this does not work, so control-C is less useful
* there.
*/
-volatile bool sigint_interrupt_enabled = false;
+volatile sig_atomic_t sigint_interrupt_enabled = false;
sigjmp_buf sigint_interrupt_jmp;
diff --git a/src/bin/psql/common.h b/src/bin/psql/common.h
index d84c3a007c..f0820dd7d5 100644
--- a/src/bin/psql/common.h
+++ b/src/bin/psql/common.h
@@ -9,6 +9,7 @@
#define COMMON_H
#include <setjmp.h>
+#include <signal.h>
#include "fe_utils/print.h"
#include "fe_utils/psqlscan.h"
@@ -22,7 +23,7 @@ extern char *psql_get_variable(const char *varname, PsqlScanQuoteType quote,
extern void NoticeProcessor(void *arg, const char *message);
-extern volatile bool sigint_interrupt_enabled;
+extern volatile sig_atomic_t sigint_interrupt_enabled;
extern sigjmp_buf sigint_interrupt_jmp;
diff --git a/src/include/common/string.h b/src/include/common/string.h
index 3d59172151..598b4d0fd6 100644
--- a/src/include/common/string.h
+++ b/src/include/common/string.h
@@ -10,13 +10,15 @@
#ifndef COMMON_STRING_H
#define COMMON_STRING_H
+#include <signal.h>
+
struct StringInfoData; /* avoid including stringinfo.h here */
typedef struct PromptInterruptContext
{
/* To avoid including <setjmp.h> here, jmpbuf is declared "void *" */
void *jmpbuf; /* existing longjmp buffer */
- volatile bool *enabled; /* flag that enables longjmp-on-interrupt */
+ volatile sig_atomic_t *enabled; /* flag that enables longjmp-on-interrupt */
bool canceled; /* indicates whether cancellation occurred */
} PromptInterruptContext;
--
2.27.0
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