Re: Rename setup_cancel_handler in pg_dump
On Wed, 7 Feb 2024 22:59:48 +0100 Daniel Gustafsson wrote: > > On 1 Feb 2024, at 02:21, Yugo NAGATA wrote: > > On Tue, 30 Jan 2024 13:44:28 +0100 > > Daniel Gustafsson wrote: > > >> -setup_cancel_handler(void) > >> +pg_dump_setup_cancel_handler(void) > >> > >> We don't have any other functions prefixed with pg_dump_, based on the > >> naming > >> of the surrounding code in the file I wonder if set_cancel_handler is a > >> more > >> appropriate name? > > > > Agreed. Here is a updated patch. > > Sleeping on it I still think this is a good idea, and hearing no objections I > went ahead with this. Thanks for the patch! Thank you! Regards, Yugo Nagata > > -- > Daniel Gustafsson > -- Yugo NAGATA
Re: Rename setup_cancel_handler in pg_dump
> On 1 Feb 2024, at 02:21, Yugo NAGATA wrote: > On Tue, 30 Jan 2024 13:44:28 +0100 > Daniel Gustafsson wrote: >> -setup_cancel_handler(void) >> +pg_dump_setup_cancel_handler(void) >> >> We don't have any other functions prefixed with pg_dump_, based on the naming >> of the surrounding code in the file I wonder if set_cancel_handler is a more >> appropriate name? > > Agreed. Here is a updated patch. Sleeping on it I still think this is a good idea, and hearing no objections I went ahead with this. Thanks for the patch! -- Daniel Gustafsson
Re: Rename setup_cancel_handler in pg_dump
On Tue, 30 Jan 2024 13:44:28 +0100
Daniel Gustafsson wrote:
> > On 26 Jan 2024, at 01:42, Yugo NAGATA wrote:
>
> > I am proposing it because there is a public function with
> > the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
> > does not include fe_utils/cancel.h, so there is no conflict,
> > but I think it is better to use different names to reduce
> > possible confusion.
>
> Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with
> other frontend utils, I can see the risk of confusion.
Thank you for looking into it!
> -setup_cancel_handler(void)
> +pg_dump_setup_cancel_handler(void)
>
> We don't have any other functions prefixed with pg_dump_, based on the naming
> of the surrounding code in the file I wonder if set_cancel_handler is a more
> appropriate name?
Agreed. Here is a updated patch.
Regards,
Yugo Nagata
>
> --
> Daniel Gustafsson
>
--
Yugo NAGATA
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..a09247fae4 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void set_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that set_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+set_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+set_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ set_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
Re: Rename setup_cancel_handler in pg_dump
> On 26 Jan 2024, at 01:42, Yugo NAGATA wrote: > I am proposing it because there is a public function with > the same name in fe_utils/cancel.c. I know pg_dump/parallel.c > does not include fe_utils/cancel.h, so there is no conflict, > but I think it is better to use different names to reduce > possible confusion. Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with other frontend utils, I can see the risk of confusion. -setup_cancel_handler(void) +pg_dump_setup_cancel_handler(void) We don't have any other functions prefixed with pg_dump_, based on the naming of the surrounding code in the file I wonder if set_cancel_handler is a more appropriate name? -- Daniel Gustafsson
Rename setup_cancel_handler in pg_dump
Hi,
Attached is a simple patch to rename setup_cancel_handler()
in pg_dump/parallel.c.
I am proposing it because there is a public function with
the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
does not include fe_utils/cancel.h, so there is no conflict,
but I think it is better to use different names to reduce
possible confusion.
I guess there was no concerns when setup_cancel_handler in
pg_dump/parallel.c was introduced because the same name
function was not in fe_utils that could be used in common
between client tools.. The public setup_cancel_handler in
fe_utils was introduced in a4fd3aa719e, where this function
was moved from psql.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..261b23cb3f 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void pg_dump_setup_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that pg_dump_setup_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ pg_dump_setup_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
